Closed
Bug 239132
Opened 20 years ago
Closed 20 years ago
nsIWidget::SetTitle should use nsAString, not nsString
Categories
(Core Graveyard :: GFX, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mpgritti, Unassigned)
References
Details
Attachments
(1 file, 5 obsolete files)
23.40 KB,
patch
|
Details | Diff | Splinter Review |
nsIWidget.h is used by all embedding clients. Including nsString.h cause conflicts with nsEmbedString.h (see 238088).
Comment 1•20 years ago
|
||
hmm... nsIWidget.h is not part of the Gecko SDK. it sounds like you are also saying that it needs to be included or should be included. since nsIWidget actually has method(s) that use nsString references, this change requires an interface change.
Comment 2•20 years ago
|
||
Why do embedding clients need nsIWidget? That's an internal "interface" as far as I am aware.... Perhaps the needed functionality should be exposed in a freezable way?
Reporter | ||
Comment 3•20 years ago
|
||
darin, yeah as per bug title the only method to change would be SetTitle. nsIWidget is listed in http://www.mozilla.org/projects/embedding/EmbedInterfaceFreeze.html but only in the "api that embedding clients are using", so I'm not sure if it's meant to be part of the SDK. Since the preferred class to use in methods in nsAString maybe it should be fixed anyway. Though, admittely it could be a pain because it needs changes to all widget implementations. bzbarsky. The embedding clients seem to use it for misc reasons. gtkmozembed use it to access the native data (the GdkWindow) to show tooltips and on realize. I'll see if I can find alternatives.
Comment 4•20 years ago
|
||
I agree with BZ. nsIWidget is probably not at all what we want to expose to embedders. gtkmozembed is a module in the mozilla codebase, so it therefore can play with any interface it likes since it is always updated when mozilla is updated. i wouldn't look at what gtkmozembed uses internally to decide what other external apps can/should use. there may be a better way. mfcembed might be a better reference in some cases.
Reporter | ||
Comment 5•20 years ago
|
||
I dont disagree with BZ. Though we still need a solution to make 205425 possible. If nsIWidget.h depend on mozilla internal string implementation then an alternative to the use of nsIWidget.h need to be provided. The ability to build an embedding application on linux, with or without gtkmozembed, shouldnt depend on apis that are considered internal. My point is not that we should consider public the interfaces gtkmozembed uses but that we need a way to make gtkmozembed (or something similar) possible without using internal apis. Since apparently there is consensus that nsIWidget.h should not be considered public, I'm going to close this and investigate the possibility of getting rid of nsIWidget in gtkmozembed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → INVALID
Reporter | ||
Updated•20 years ago
|
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Reporter | ||
Comment 6•20 years ago
|
||
Reopened after discussing it in irc with Darin. It make sense to do this single change to ease up a bit the process of gtkmozembed GRE porting. It doesnt really mean nsIWidget has to be considered a to-freeze interface.
Reporter | ||
Comment 7•20 years ago
|
||
There are very likely problems, admittely this goes way beyhond my current mozilla knowledge :/
Reporter | ||
Updated•20 years ago
|
Attachment #145515 -
Flags: review?(bsmedberg)
Reporter | ||
Comment 8•20 years ago
|
||
Attachment #145515 -
Attachment is obsolete: true
Reporter | ||
Updated•20 years ago
|
Attachment #145515 -
Flags: review?(bsmedberg)
Reporter | ||
Updated•20 years ago
|
Attachment #145520 -
Flags: review?(bsmedberg)
Comment 9•20 years ago
|
||
Comment on attachment 145520 [details] [diff] [review] Use PromiseFlatString and fix naming conflicts in windows/ >Index: src/gtk/nsWindow.cpp >- PRInt32 len = (PRInt32)aTitle.Length(); >- encoder->GetMaxLength(aTitle.get(), len, &platformLen); >+ const nsString& title = PromiseFlatString(aTitle); >+ PRInt32 len = (PRInt32)title.Length(); >+ encoder->GetMaxLength(title.get(), len, &platformLen); The nsIUnicodeEncoder API does not require a flat string. You can use BeginReading to get a pointer to the start of the nsACString's buffer instead. Something like this should work: nsACString::const_iterator begin; const char *title = aTitle.BeginReading(begin).get(); A bit awkward I admit :-/ >+NS_IMETHODIMP nsMacWindow::SetTitle(const nsAString& aTitle) > { >+ const nsString& strTitle = PromiseFlatString(aTitle); > #if TARGET_CARBON > if(nsToolkit::OnMacOSX()) { > // On MacOS X try to use the unicode friendly CFString version first >+ CFStringRef labelRef = ::CFStringCreateWithCharacters(kCFAllocatorDefault, (UniChar*)strTitle.get(), strTitle.Length()); CFStringCreateWithCharacters also doesn't require the input array to be null terminated. >Index: src/xlib/nsWindow.cpp >- PRInt32 len = (PRInt32)aTitle.Length(); >- encoder->GetMaxLength(aTitle.get(), len, &platformLen); >+ const nsString& title = PromiseFlatString(aTitle); >+ PRInt32 len = (PRInt32)title.Length(); >+ encoder->GetMaxLength(title.get(), len, &platformLen); same nit here. sr=darin with those changes and assuming bsmedberg is happy with the patch.
Attachment #145520 -
Flags: superreview+
Comment 10•20 years ago
|
||
Comment on attachment 145520 [details] [diff] [review] Use PromiseFlatString and fix naming conflicts in windows/ >Index: src/windows/nsWindow.cpp >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v >retrieving revision 3.494 >diff -u -r3.494 nsWindow.cpp >--- src/windows/nsWindow.cpp 1 Apr 2004 22:51:46 -0000 3.494 >+++ src/windows/nsWindow.cpp 6 Apr 2004 00:16:16 -0000 >@@ -5539,11 +5539,12 @@ > return acp; > } > >-NS_METHOD nsWindow::SetTitle(const nsString& aTitle) >+NS_METHOD nsWindow::SetTitle(const nsAString& aTitle) > { >- char* title = GetACPString(aTitle); >+ const nsString& strTitle = PromiseFlatString(aTitle); >+ char* title = GetACPString(strTitle); > if (title) { >- nsToolkit::mSendMessage(mWnd, WM_SETTEXT, (WPARAM)0, (LPARAM)(LPCWSTR)PromiseFlatString(aTitle).get()); >+ nsToolkit::mSendMessage(mWnd, WM_SETTEXT, (WPARAM)0, (LPARAM)(LPCWSTR)strTitle.get()); > delete [] title; > } > return NS_OK; Get rid of "title" and the ACPString conversion altogether; it's a relic from the pre-MOZ_UNICODE middle ages. Can you please make another patch with these nits fixed... I will apply it to mac to make sure we can build.
Attachment #145520 -
Flags: review?(bsmedberg) → review-
Reporter | ||
Comment 11•20 years ago
|
||
This includes requested changes apart the mac/ one. The problem there is that I need an nsString to pass to nsMacControl::StringToStr255 at the end of the function. So I guess it make sense to use it in the CARBON ifdef too. I'm going to attach another version of this patch that make StringToStr255 take nsAString instead. That start to be pretty invasive though ...
Reporter | ||
Updated•20 years ago
|
Attachment #145520 -
Attachment is obsolete: true
Reporter | ||
Comment 12•20 years ago
|
||
Reporter | ||
Comment 13•20 years ago
|
||
Attachment #145538 -
Attachment is obsolete: true
Reporter | ||
Updated•20 years ago
|
Attachment #145569 -
Flags: review?(bsmedberg)
Comment 14•20 years ago
|
||
Comment on attachment 145569 [details] [diff] [review] fix nits (with mac/ changes). Actuallly change mac impl of SetTitle as darin requested. r=me carrying over darin's sr asking for approval... I'll apply this on mac sometime tomorrow
Attachment #145569 -
Flags: superreview+
Attachment #145569 -
Flags: review?(bsmedberg)
Attachment #145569 -
Flags: review+
Attachment #145569 -
Flags: approval1.7?
Reporter | ||
Comment 15•20 years ago
|
||
Sorry, the only change is -#include "nsString.h" in nsIWidget.h
Attachment #145536 -
Attachment is obsolete: true
Attachment #145569 -
Attachment is obsolete: true
Reporter | ||
Comment 16•20 years ago
|
||
The patch was checked in
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Comment 17•20 years ago
|
||
Comment on attachment 145569 [details] [diff] [review] fix nits (with mac/ changes). Actuallly change mac impl of SetTitle as darin requested. a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145569 -
Flags: approval1.7? → approval1.7+
Updated•20 years ago
|
Attachment #145569 -
Flags: approval1.7+
Updated•15 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•