Closed Bug 239132 Opened 20 years ago Closed 20 years ago

nsIWidget::SetTitle should use nsAString, not nsString

Categories

(Core Graveyard :: GFX, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mpgritti, Unassigned)

References

Details

Attachments

(1 file, 5 obsolete files)

nsIWidget.h is used by all embedding clients. Including nsString.h cause
conflicts with nsEmbedString.h (see 238088).
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.
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?
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.
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.
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
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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.
Attached patch Use nsAString (obsolete) — Splinter Review
There are very likely problems, admittely this goes way beyhond my current
mozilla knowledge :/
Attachment #145515 - Flags: review?(bsmedberg)
Attachment #145515 - Attachment is obsolete: true
Attachment #145515 - Flags: review?(bsmedberg)
Attachment #145520 - Flags: review?(bsmedberg)
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 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-
Attached patch fix nits (no mac/ changes) (obsolete) — Splinter Review
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 ...
Attachment #145520 - Attachment is obsolete: true
Attached patch fix nits (with mac/ changes) (obsolete) — Splinter Review
Blocks: 239789
Attachment #145569 - Flags: review?(bsmedberg)
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?
Sorry, the only change is -#include "nsString.h" in nsIWidget.h
Attachment #145536 - Attachment is obsolete: true
Attachment #145569 - Attachment is obsolete: true
The patch was checked in
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
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+
Attachment #145569 - Flags: approval1.7+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.