Last Comment Bug 239132 - nsIWidget::SetTitle should use nsAString, not nsString
: nsIWidget::SetTitle should use nsAString, not nsString
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: GFX (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: general
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 239789
  Show dependency treegraph
 
Reported: 2004-03-29 16:28 PST by Marco Pesenti Gritti
Modified: 2009-01-22 10:17 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use nsAString (21.38 KB, patch)
2004-04-05 16:24 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
Use PromiseFlatString and fix naming conflicts in windows/ (21.46 KB, patch)
2004-04-05 17:22 PDT, Marco Pesenti Gritti
benjamin: review-
darin.moz: superreview+
Details | Diff | Splinter Review
fix nits (no mac/ changes) (21.58 KB, patch)
2004-04-06 01:53 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
fix nits (with mac/ changes) (23.44 KB, patch)
2004-04-06 02:03 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
fix nits (with mac/ changes). Actuallly change mac impl of SetTitle as darin requested. (23.21 KB, patch)
2004-04-06 16:05 PDT, Marco Pesenti Gritti
benjamin: review+
benjamin: superreview+
Details | Diff | Splinter Review
actually remove the nsString inclusion (23.40 KB, patch)
2004-04-12 17:03 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review

Description Marco Pesenti Gritti 2004-03-29 16:28:28 PST
nsIWidget.h is used by all embedding clients. Including nsString.h cause
conflicts with nsEmbedString.h (see 238088).
Comment 1 Darin Fisher 2004-03-29 19:18:37 PST
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 [:bz] 2004-03-29 21:56:57 PST
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 2004-03-30 00:24:18 PST
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 2004-03-30 06:51:57 PST
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 2004-03-30 07:22:18 PST
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 2004-04-01 11:49:59 PST
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 2004-04-05 16:24:07 PDT
Created attachment 145515 [details] [diff] [review]
Use nsAString

There are very likely problems, admittely this goes way beyhond my current
mozilla knowledge :/
Comment 8 Marco Pesenti Gritti 2004-04-05 17:22:32 PDT
Created attachment 145520 [details] [diff] [review]
Use PromiseFlatString and fix naming conflicts in windows/
Comment 9 Darin Fisher 2004-04-05 20:50:36 PDT
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 [:bsmedberg] 2004-04-05 22:46:20 PDT
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 2004-04-06 01:53:07 PDT
Created attachment 145536 [details] [diff] [review]
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 2004-04-06 02:03:58 PDT
Created attachment 145538 [details] [diff] [review]
fix nits (with mac/ changes)
Comment 13 Marco Pesenti Gritti 2004-04-06 16:05:25 PDT
Created attachment 145569 [details] [diff] [review]
fix nits (with mac/ changes). Actuallly change mac impl of SetTitle as darin requested.
Comment 14 Benjamin Smedberg [:bsmedberg] 2004-04-06 17:41:22 PDT
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 2004-04-12 17:03:00 PDT
Created attachment 145958 [details] [diff] [review]
actually remove the nsString inclusion

Sorry, the only change is -#include "nsString.h" in nsIWidget.h
Comment 16 Marco Pesenti Gritti 2004-04-19 15:29:35 PDT
The patch was checked in
Comment 17 Asa Dotzler [:asa] 2004-04-28 16:10:37 PDT
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

Note You need to log in before you can comment on or make changes to this bug.