239132 - nsIWidget::SetTitle should use nsAString, not nsString

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Marco Pesenti Gritti]

nsIWidget.h is used by all embedding clients. Including nsString.h cause
conflicts with nsEmbedString.h (see 238088).

Comment 1

[Darin Fisher]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Marco Pesenti Gritti]

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

[Darin Fisher]

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.

Comment 5

[Marco Pesenti Gritti]

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.

Comment 6

[Marco Pesenti Gritti]

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.

Comment 7

[Marco Pesenti Gritti]

Attachment: Use nsAString

There are very likely problems, admittely this goes way beyhond my current
mozilla knowledge :/

Comment 8

[Marco Pesenti Gritti]

Attachment: Use PromiseFlatString and fix naming conflicts in windows/

Comment 9

[Darin Fisher]

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.

Comment 10

[Benjamin Smedberg]

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.

Comment 11

[Marco Pesenti Gritti]

Attachment: fix nits (no mac/ changes)

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 ...

Comment 12

[Marco Pesenti Gritti]

Attachment: fix nits (with mac/ changes)

Comment 13

[Marco Pesenti Gritti]

Attachment: fix nits (with mac/ changes). Actuallly change mac impl of SetTitle as darin requested.

Comment 14

[Benjamin Smedberg]

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

Comment 15

[Marco Pesenti Gritti]

Attachment: actually remove the nsString inclusion

Sorry, the only change is -#include "nsString.h" in nsIWidget.h

Comment 16

[Marco Pesenti Gritti]

The patch was checked in

Comment 17

[Asa Dotzler [:asa]]

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