nsIWidget::SetTitle should use nsAString, not nsString

RESOLVED FIXED

Status

Core Graveyard
GFX
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: Marco Pesenti Gritti, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

14 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

14 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

14 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
Last Resolved: 14 years ago
Resolution: --- → INVALID
(Reporter)

Updated

14 years ago
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Reporter)

Comment 6

14 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

14 years ago
Created attachment 145515 [details] [diff] [review]
Use nsAString

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

Updated

14 years ago
Attachment #145515 - Flags: review?(bsmedberg)
(Reporter)

Comment 8

14 years ago
Created attachment 145520 [details] [diff] [review]
Use PromiseFlatString and fix naming conflicts in windows/
Attachment #145515 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #145515 - Flags: review?(bsmedberg)
(Reporter)

Updated

14 years ago
Attachment #145520 - Flags: review?(bsmedberg)

Comment 9

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

14 years ago
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 ...
(Reporter)

Updated

14 years ago
Attachment #145520 - Attachment is obsolete: true
(Reporter)

Comment 12

14 years ago
Created attachment 145538 [details] [diff] [review]
fix nits (with mac/ changes)
(Reporter)

Updated

14 years ago
Blocks: 239789
(Reporter)

Comment 13

14 years ago
Created attachment 145569 [details] [diff] [review]
fix nits (with mac/ changes). Actuallly change mac impl of SetTitle as darin requested.
Attachment #145538 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
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?
(Reporter)

Comment 15

14 years ago
Created attachment 145958 [details] [diff] [review]
actually remove the nsString inclusion

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

14 years ago
The patch was checked in
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 17

14 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

14 years ago
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.