Closed Bug 370698 Opened 13 years ago Closed 11 years ago

Port Firefox utilityOverlay.js API to SeaMonkey (SuiteRunner)

Categories

(SeaMonkey :: General, defect, P5)

defect

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a2

People

(Reporter: asrail, Assigned: asrail)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 19 obsolete files)

8.74 KB, patch
asrail
: review+
asrail
: superreview+
Details | Diff | Splinter Review
There are some useful functions on the FX's utilityOverlay.js (such as openLinkUI) that are used by a lot of extensions.

It would reduce a lot the need to port extensions from/to SeaMonkey after the move to toolkit.

I'm not proposing to break the current SM's utilityOverlay.js API, but some studies should be made in the direction of unifying both utilityOverlay.js's (and others).
Off course it would be better to add something relevant to Firefox (on FX 3) than removing from SM (the demand to migrate SM specific extensions that would work on FX is almost zero, anyway).
Status: NEW → ASSIGNED
Priority: -- → P3
Version: unspecified → Trunk
There are at least three use cases for implementing openUILinkIn/openUILinkIn in SeaMonkey.

1. Several extensions including Forecastfox use these convenience functions.
2. The EM manager (which SuiteRunner uses) tries to use this to open links. Except that it doesn't because it thinks it's in Thunderbird. But if this bug is fixed then having openUILink would be needed.
3. Having this available would make it easier for Chatzilla to *not* open links in the current tab.
4. SM only has such a method (handy to opening links) inside content area.
5. That is not a bad refactoring.


openUILinkIn is the main purpose of this bug, but I'm analyzing the other methods too.

I would simplify:
porting such APIs will make SM a lot more compatible with FX extensions.


It's possible to "copy'n'past" and to threat exceptions to implement openUILink (and requisites) on extensions, but would be a lot better to install the original extension from AMO instead of porting.

I would like to see the same effort on other places, if they be shown as needed.


About breaking existing APIs...
should Suiterunner be the time to rename: "goAboutDialog" to "openAboutDialog" and "goReleaseNotes" to "openReleaseNotes"?

Not only FX uses the later, but SM already uses "openPreferences" and "open" makes more sense here.

I can leave it alone if it to be shown controversial.
But since this is a *major* version change, it would be better to break it now than later.


Lets ignore for the moment functions that are differently named or behave differently (and are likely to run into review objections) and go for the low hanging fruit first. That is functions that [1] don't have any equivalent in SeaMonkey and [2] are often used by Firefox extensions.
Philip, I'm not requesting this change to the non-Suiterunner SM yet, so these changes won't be visible to the current trunk.

There are a few changes, such as making openUILink to return the window because openTopWin behaves this way.


No renaming of functions neither method signature to the existing ones (FX has a openNewTabWith with a method signature very distinct to the SM one...).
Attachment #255835 - Flags: review?
Attachment #255835 - Flags: review? → review?(timeless)
Attachment #255835 - Attachment is obsolete: true
Attachment #255846 - Flags: review?(cst)
Attachment #255835 - Flags: review?(timeless)
> +function getBoolPref( prefname, def )
Can we leverage on the implementation already present in nsUserSettings.js?

> +  switch (where) {
> +  case "current":
> +    w.loadURI(url, null, postData, allowThirdPartyFixup);
The Suite implementation of loadURI is different:
<http://lxr.mozilla.org/seamonkey/source/suite/browser/navigator.js#1468>
i.e. function loadURI(uri, referrer, flags)
If you want to change loadURI then you will need to change all the callers.

> +function checkForMiddleClick( node, event )
Not used by extensions in my experience. The only use case for Suite that I can see is if the safebrowsing extension is included with the suite.

> +function closeMenus( node )
Helper function only used by checkForMiddleClick()

> +function isElementVisible( aElement )
Not used by extensions in my experience. Convenience function only used internally by Firefox.
(In reply to comment #7)

Thank you a lot for your comments Philip. I don't have such a experience in converting extensions to SM.

> > +function getBoolPref( prefname, def )
> Can we leverage on the implementation already present in nsUserSettings.js?

Well... we can.

> > +  switch (where) {
> > +  case "current":
> > +    w.loadURI(url, null, postData, allowThirdPartyFixup);
> The Suite implementation of loadURI is different:
> <http://lxr.mozilla.org/seamonkey/source/suite/browser/navigator.js#1468>
> i.e. function loadURI(uri, referrer, flags)
> If you want to change loadURI then you will need to change all the callers.

That worked and I've forgot to check the method signature of this and loadOneTab (addTab, though).

> > +function checkForMiddleClick( node, event )
> Not used by extensions in my experience. The only use case for Suite that I can
> see is if the safebrowsing extension is included with the suite.

It would be possible...

> > +function closeMenus( node )
> Helper function only used by checkForMiddleClick()
> 
> > +function isElementVisible( aElement )
> Not used by extensions in my experience. Convenience function only used
> internally by Firefox.
> 

I've seen that it's used by Firefox only to look at the URL bar and use the open URL dialog when it's hidden (SM Open Web Location), but I didn't know about how popular it was in the extension development.

I'll remove these non highly requested methods.

In the toolkit nsUserSettings.js is in "obsolete", that may be bad.

Also I've seen some extensions using getBoolPref, I believe it's widely used and I hope you don't meant we should to remove it.
Attachment #255846 - Attachment is obsolete: true
Attachment #255867 - Flags: review?(cst)
Attachment #255846 - Flags: review?(cst)
> In the toolkit nsUserSettings.js is in "obsolete", that may be bad.
Ah my bad. I didn't know that.

> Also I've seen some extensions using getBoolPref, I believe it's
> widely used and I hope you don't meant we should to remove it.
No I just thought we should avoid duplicating code, but:

> +function getBoolPref( prefname, def )
> +{
> +  return nsPreferences.getBoolPref(prefname, def);
I just raised it as a possibility for discussion, I didn't mean for you to actually do it! For one thing: is nsUserSettings.js always available to utilityOverlay.js? Sorry for the misunderstanding. I think it might be best to revert to a standalone implementation.
> +  var flags = nsIWebNavigation.LOAD_FLAGS_NONE;
> +  if (aAllowThirdPartyFixup)
> +    flags |= nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;

I'm not familiar with these but isn't
LOAD_FLAGS_NONE | LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
exactly the same as just LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP?
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIWebNavigation.idl#117>

var flags = aAllowThirdPartyFixup ? nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP : nsIWebNavigation.LOAD_FLAGS_NONE;
Attached patch Making suggested changes (obsolete) — Splinter Review
Reverting getBoolPref to the old implementation, using the short-circuit proposed by Philip and fixed a typo.
Attachment #255867 - Attachment is obsolete: true
Attachment #255890 - Flags: review?(cst)
Attachment #255867 - Flags: review?(cst)
> +  var flags = allowThirdPartyFixup ? nsIWebNavigation.LOAD_FLAGS_NONE :
> +    nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
Um, are you sure the order is correct?

> +  if (!w || where == "window") {
> +    return openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url,
> +                      null, null, postData, flags);
<http://lxr.mozilla.org/seamonkey/source/suite/browser/navigator.js#663>
In SuiteRunner, I believe that the order of window.arguments is: [url, turbo, referrer, flags] and "postData" isn't defined anywhere in your patch that I can see. Sorry I should have spotted this earlier.
Attached patch Minor fixes (obsolete) — Splinter Review
(In reply to comment #13)
> > +  var flags = allowThirdPartyFixup ? nsIWebNavigation.LOAD_FLAGS_NONE :
> > +    nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> Um, are you sure the order is correct?

I'm sure it isn't.

> > +  if (!w || where == "window") {
> > +    return openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url,
> > +                      null, null, postData, flags);
> <http://lxr.mozilla.org/seamonkey/source/suite/browser/navigator.js#663>
> In SuiteRunner, I believe that the order of window.arguments is: [url, turbo,
> referrer, flags] and "postData" isn't defined anywhere in your patch that I can
> see. Sorry I should have spotted this earlier.
> 

postData was removed because that SM API won't use it. addTab would have to accept it as argument, but today it just uses null.
I've removed this garbage from the patch.
Attachment #255890 - Attachment is obsolete: true
Attachment #255890 - Flags: review?(cst)
Attachment #255902 - Flags: review?(cst)
If the attachment of bug 361303 land in before this one, another patch will be required here.
I believe this will be the case.

Comment on attachment 255902 [details] [diff] [review]
Minor fixes

I haven't tested the patch yet.  I also didn't look at the new toolkit functions in detail because I'm assuming the toolkit people got it right (but I will have to look at the next version to see if they call anything external that needs to be looked at).

>@@ -300,26 +285,26 @@ function goClickThrobber( urlPref )
> 
>   if ( url )
>-    openTopWin(url);
>+    openUILinkIn(url);

openUILinkIn doesn't do anything if you don't pass more arguments.  I think you mean openUILink()

>+// Method added for API compatibility with Firefox
>+function getBoolPref( prefname, def )

Can't we just include toolkit's nsUserSettings.js to get the whole thing?  After all, the file is part of toolkit/, not browser/.

>+function focusElement( aElement )
>+function openUILink( url, e, ignoreButton, ignoreAlt, allowKeywordFixup )
>+function whereToOpenLink( e, ignoreButton, ignoreAlt )
>+function openUILinkIn( url, where, allowThirdPartyFixup )

Did you modify these at all from the Firefox version? I'm assuming you did, since I don't see:

163 #ifdef XP_MACOSX
164   if (meta || (middle && middleUsesTabs)) {
165 #else
166   if (ctrl || (middle && middleUsesTabs)) {
167 #endif

What does your patch do on mac?  (FYI, tabbrowser.xml has a /Mac/.test() example that you could use to do platform-dependent stuff).
Attachment #255902 - Flags: review?(cst) → review-
Attached patch Fixes... (obsolete) — Splinter Review
(In reply to comment #16)
> (From update of attachment 255902 [details] [diff] [review])
> I haven't tested the patch yet.  I also didn't look at the new toolkit
> functions in detail because I'm assuming the toolkit people got it right (but I
> will have to look at the next version to see if they call anything external
> that needs to be looked at).
> 
> >@@ -300,26 +285,26 @@ function goClickThrobber( urlPref )
> > 
> >   if ( url )
> >-    openTopWin(url);
> >+    openUILinkIn(url);
> 
> openUILinkIn doesn't do anything if you don't pass more arguments.  I think you
> mean openUILink()

I node...

> >+// Method added for API compatibility with Firefox
> >+function getBoolPref( prefname, def )
> 
> Can't we just include toolkit's nsUserSettings.js to get the whole thing? 
> After all, the file is part of toolkit/, not browser/.

I've used that in one of the previous ones, I haven't liked that file living under the obsolete directory. But if that doesn't mean anything bad, yes, we can.

> >+function focusElement( aElement )
> >+function openUILink( url, e, ignoreButton, ignoreAlt, allowKeywordFixup )
> >+function whereToOpenLink( e, ignoreButton, ignoreAlt )
> >+function openUILinkIn( url, where, allowThirdPartyFixup )
> 
> Did you modify these at all from the Firefox version? I'm assuming you did,
> since I don't see:
> 
> 163 #ifdef XP_MACOSX
> 164   if (meta || (middle && middleUsesTabs)) {
> 165 #else
> 166   if (ctrl || (middle && middleUsesTabs)) {
> 167 #endif

Sorry...

> What does your patch do on mac?  (FYI, tabbrowser.xml has a /Mac/.test()
> example that you could use to do platform-dependent stuff).
> 

I've added utilityOverlay.js to the text pre-processor and I've used the same thing as in FX's one...
Attachment #255902 - Attachment is obsolete: true
Attachment #260935 - Flags: review?(cst)
Attachment #260935 - Attachment is obsolete: true
Attachment #260936 - Flags: review?(cst)
Attachment #260935 - Flags: review?(cst)
Comment on attachment 260936 [details] [diff] [review]
A very minor fix over the last one (sorry)...

(In reply to comment #17)
> Created an attachment (id=260935) [details]
> Fixes...
> 
> (In reply to comment #16)
> > (From update of attachment 255902 [details] [diff] [review] [details])
> > openUILinkIn doesn't do anything if you don't pass more arguments.  I think you
> > mean openUILink()
> 
> I node...

I wonder why openUILinkIn fails silently rather than throwing.

> > >+// Method added for API compatibility with Firefox
> > >+function getBoolPref( prefname, def )
> > 
> > Can't we just include toolkit's nsUserSettings.js to get the whole thing? 
> > After all, the file is part of toolkit/, not browser/.
> 
> I've used that in one of the previous ones, I haven't liked that file living
> under the obsolete directory. But if that doesn't mean anything bad, yes, we
> can.

Neil, thoughts?

I'm concerned about the use of nsPreferences - is it really available from all users of utilityOverlay.js (e.g. view source)?  I think it will fail silently.
Attachment #260936 - Flags: review?(cst) → review-
Comment on attachment 260936 [details] [diff] [review]
A very minor fix over the last one (sorry)...

Oh, and sorry for taking forever to review this.
Attachment #260936 - Attachment is obsolete: true
Attachment #263215 - Flags: superreview?(neil)
Attachment #263215 - Flags: review?(cst)
Comment on attachment 263215 [details] [diff] [review]
Don't leveraging on nsPreferences.

>-   content/communicator/utilityOverlay.js
>+*  content/communicator/utilityOverlay.js
Does this need to live in utilityOverlay.js or can it live in contentAreaUtils.js? Also see below.

>+  // We are resizable ONLY if in box debugging mode, because in
>+  // this special debug mode it is often impossible to see the 
>+  // content of the debug panel in order to disable debug mode.
>+  var resizable = getBoolPref("xul.debug.box", false);
>   var pref = Components.classes["@mozilla.org/preferences-service;1"]
>                        .getService(Components.interfaces.nsIPrefBranch);
You forgot to remove pref too.

>+    toolbar.hidden = !attribValue;
>     document.persist(id, 'hidden');
This change doesn't work, because .hidden can remove the attribute, so that there's nothing to persist.

>+#ifdef XP_MACOSX
>+  if (meta || (middle && middleUsesTabs)) {
>+#else
>+  if (ctrl || (middle && middleUsesTabs)) {
>+#endif
Suite just uses meta || ctrl

>+  var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground", false);
We don't have this pref.

>+  // Call focusElement(w.content) instead of w.content.focus() to make sure
>+  // that we don't raise the old window, since the URI we just loaded may have
>+  // resulted in a new frontmost window (e.g. "javascript:window.open('');").
>+  
>+  focusElement(w.content);
What if the URI doesn't result in a new window?
Attachment #263215 - Flags: superreview?(neil) → superreview-
> Does this need to live in utilityOverlay.js or can it live in
> contentAreaUtils.js? Also see below.

As well as adding a bunch of functions from Firefox, Asrail's patch also modifies existing functions in SeaMonkey's utilityOverlay.js to leverage on these new functions, so moving them to contentAreaUtils.js may be problematical.

Extensions that call openUILinkIn() will likely only live in browser.xul (or in SM's case navigator.xul) or else load utilityOverlay.js explicitly, and if they do in the case of Firefox they they would also need to load contentAreaUtils.js so no change if an extension is ported to SeaMonkey. Also the |saveURL| code path isn't invoked by existing SeaMonkey code as far as I can see.

>>+#ifdef XP_MACOSX
>>+  if (meta || (middle && middleUsesTabs)) {
>>+#else
>>+  if (ctrl || (middle && middleUsesTabs)) {
>>+#endif

Should the preprocessor be invoked for just these lines? Is there any disadvantage of using |/Mac/.test(navigator.platform)| instead?

>>+  var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground", false);
>We don't have this pref.

How about using "browser.tabs.loadInBackground" instead?

>>+  focusElement(w.content);
>What if the URI doesn't result in a new window?

Won't focusElement() focus on the current window then?
(In reply to comment #22)
> (From update of attachment 263215 [details] [diff] [review])
> >-   content/communicator/utilityOverlay.js
> >+*  content/communicator/utilityOverlay.js
> Does this need to live in utilityOverlay.js or can it live in
> contentAreaUtils.js? Also see below.

I agreed with Philip.
I won't request the pre-processing change anymore.

> >+  // We are resizable ONLY if in box debugging mode, because in
> >+  // this special debug mode it is often impossible to see the 
> >+  // content of the debug panel in order to disable debug mode.
> >+  var resizable = getBoolPref("xul.debug.box", false);
> >   var pref = Components.classes["@mozilla.org/preferences-service;1"]
> >                        .getService(Components.interfaces.nsIPrefBranch);
> You forgot to remove pref too.

... thanks.

> >+    toolbar.hidden = !attribValue;
> >     document.persist(id, 'hidden');
> This change doesn't work, because .hidden can remove the attribute, so that
> there's nothing to persist.

OK.

> >+#ifdef XP_MACOSX
> >+  if (meta || (middle && middleUsesTabs)) {
> >+#else
> >+  if (ctrl || (middle && middleUsesTabs)) {
> >+#endif
> Suite just uses meta || ctrl

Done.

> >+  var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground", false);
> We don't have this pref.

I've taken it as loadInBackground, sorry.

> >+  // Call focusElement(w.content) instead of w.content.focus() to make sure
> >+  // that we don't raise the old window, since the URI we just loaded may have
> >+  // resulted in a new frontmost window (e.g. "javascript:window.open('');").
> >+  
> >+  focusElement(w.content);
> What if the URI doesn't result in a new window?
> 

It just call focus() on the content, it only does something differentt when a new window is created.

Attached patch Changes from the Neil comments (obsolete) — Splinter Review
Attachment #263215 - Attachment is obsolete: true
Attachment #265389 - Flags: review?(cst)
Attachment #265389 - Flags: superreview?(neil)
Comment on attachment 265389 [details] [diff] [review]
Changes from the Neil comments

Sorry for a) the delay in the review b) not spotting these before

>+  if (defaultAboutState == kNewWindow)
>+    target = "window";
>+  else if (defaultAboutState == kExistingWindow)
>+    target = "current";
>   else {
>+    target = "tab";
>   }
Why did you have {}s in the last case?

>+    var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                       .getService(Components.interfaces.nsIPrefBranch);
Nit: line up the .s

>+// Method added for API compatibility with Firefox
I'm not convinced by this. When we call focusElement, nothing will have started loading yet (because loading is async). So just focus the window.

>+ * On Windows, the modifiers are:
>+ * Ctrl        new tab, selected
>+ * Shift       new window
>+ * Ctrl+Shift  new tab, in background
>+ * Alt         save
Except they're not that in SeaMonkey.
If browser.tabs.opentabfor.middleclick is true, then Ctrl and Ctrl+Shift open a new tab, depending on Shift and browser.tabs.loadInBackground.
Otherwise if middlemouse.openNewWindow is true, then Ctrl and Ctrl+Shift open a new window.
Otherwise if Ctrl (or Meta) is pressed then nothing happens.
Save is Alt or Shift depending on the ui.key.saveLink.shift preference.
Otherwise if Alt is pressed then nothing happens.
Otherwise the most recent browser is used for left clicks.

>+function whereToOpenLink( e, ignoreButton, ignoreAlt )
ignoreAlt should be ignoreSave.

>+  var w = getTopWin();
Nit: don't need to do this unless where == "window"
(see some of the code you deleted)

>+  var flags = allowThirdPartyFixup ? nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP :
>+    nsIWebNavigation.LOAD_FLAGS_NONE;
Nit: line up the nsIWebNavigation.

>+    browser.addTab(url, null, null, !loadInBackground, flags);
This automatically focuses the previous tab when the tab is closed, which is inappropriate. Focus the new tab manually.
Attachment #265389 - Flags: superreview?(neil) → superreview-
Thanks for the tips.

By the way, in the meantime, I've started considering bug 384139 (moving openURL to a reusable place) instead of this.

It would be nice to be able to use it the same way across all applications, if so.
> By the way, in the meantime, I've started considering bug 384139 (moving
> openURL to a reusable place) instead of this.

? That bug is orthogonal to this.
What is this bug waiting for? Just a checkin?
No, I still need to address the Neil's comment.

I've seen that would help a few bugs around (as page info).

I've been busy last months.

Blocks: 240393
Do we still need this now that bug 384139 exposes openLink() to us from global toolkit code?
>>Do we still need this now that bug 384139 exposes openLink() to us from global
toolkit code?<<

...yes.

openUILink is different in function and concept than openLink. I'll probably implement at least that and its sibling in Bug 240393 if this doesn't land by then. (Though due to the scale of Bug 240393 I won't be doing the various extra cleanup to use the new function)
I'm actually wondering if the current behavior of openLink (and difference to openUILink) to treat those URLs as external is really the correct thing to do. Aren't all calls going to it actually internal URL calls in the case that we can handle them internally?
Caio,
Are you still working on this ?
Still working but with very low priority.

This is far from being a blocker of SM 2.0a.
Priority: P3 → P5
Could we move this forward? The code I'm using in my WIP for bug 382187 uses whereToOpenLink() and openUILinkIn() and it would be so nice if I wouldn't have to reimplement that functionality there...
This is unbitrotted, a hg patch and I tried to address the latest comments of Neil as far as I understood them. I'd like to get this in as soon as possible, but I'd be happy if someone else can drive it through the last fixes and into the tree. asrail, you still up for that?
Attachment #265389 - Attachment is obsolete: true
Attachment #341643 - Flags: superreview?(neil)
Attachment #341643 - Flags: review+
Blocks: 382187
OK, I'm taking this over and driving in as I found even more things we should add.
For one thing that's checkForMiddleClick, which is spreading fast in FF code as http://mxr.mozilla.org/comm-central/search?string=checkForMiddleClick shows and I guess it does or will spread to extensions enough that it's good for us to support it.
For the other, a function to open multiple URLs in tabs at once is something we want from history, but also from debugQA, and I guess we might e.g. want to open several feed entries in tabs at once or such stuff in the future.

As I'm adding additional stuff, requesting re-review from Neil.
Assignee: asrail → kairo
Attachment #341643 - Attachment is obsolete: true
Attachment #341662 - Flags: superreview?(neil)
Attachment #341662 - Flags: review?(neil)
Attachment #341643 - Flags: superreview?(neil)
Blocks: 458438
Comment on attachment 341662 [details] [diff] [review]
patch v2: add middleclick and URL array opening

> function openTopWin( url, opener )
So, I've no idea why I never noticed this before, but this function is actually used by View Image / Show Only This Frame code and so not only does it not actually open a UI link but the opener is important for security reasons.

>+  if (defaultAboutState == kNewWindow)
>+    target = "window";
>+  else if (defaultAboutState == kExistingWindow)
>+    target = "current";
>   else {
>+    target = "tab";
>   }
Not that I like it as an if/else if/else, but the {}s are wrong. (Maybe a switch or an array index e.g. ["current", "window", "tab"][defaultAboutState - 1])

>+     * XXX: we should use node.oncommand(event) once bug 246720 is fixed.
Do we need this function yet or can we wait until bug 246720 is fixed?

>+  if ("tagName" in node) {
I'm not sure why this was written with recursion rather than a loop, or why it bothers with a tag name check rather than just null-checking...

>+    if (node.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+    && (node.tagName == "menupopup" || node.tagName == "popup"))
node instanceof nsIDOMXULPopupElement

>+ * The logic for modifiers is as fiollowing:
Unfortunately the function then fails to follow the fiollowing (sic) logic...

>+ * I Feel Lucky
I'm (in English Google at least!)

>+    browser.addTab(url, null, null, !loadInBackground, flags);
The focus new tab parameter also links in to the restore previous tab function, is that intentional here? Normally we only use it for open link in new tab, and if we want to focus the new tab we set it as the current tab afterwards.

>+      browser.addTab(urlArray[i], null, null, !loadInBackground, flags);
Similar problem here, except it makes even less sense for tab groups, where we would focus the first tab if at all. See OpenURLArrayInTabs.
Attachment #341662 - Flags: superreview?(neil)
Attachment #341662 - Flags: superreview-
Attachment #341662 - Flags: review?(neil)
(In reply to comment #36)
> Could we move this forward? The code I'm using in my WIP for bug 382187 uses
> whereToOpenLink() and openUILinkIn() and it would be so nice if I wouldn't have
> to reimplement that functionality there...

I was thinking of extensions, not core code. I also though it was not required until the first beta, so I put more efforts on l10n side.
But I'm still willing to help here, if Kairo wants it.
I can work on it this weekend.
Caio, I'm happy with you taking this forward again, I guess I'll have enough work with getting bug 382187 through the reviews ;-)
Just move forward on my last patch here, as it has some additions to your previous works (I hope you can address Neil's current set of review comments better than me).
(In reply to comment #39)
> (From update of attachment 341662 [details] [diff] [review])
> > function openTopWin( url, opener )
> So, I've no idea why I never noticed this before, but this function is actually
> used by View Image / Show Only This Frame code and so not only does it not
> actually open a UI link but the opener is important for security reasons.

View Image / Show Only This Frame both work correctly in my builds with this path applied, from what I see. Not completely sure about the security reasons you talk about, I hope asrail can figure it out (in doubt, we can still leave this function unchanged - the important part is to get the new functions in)

> >+     * XXX: we should use node.oncommand(event) once bug 246720 is fixed.
> Do we need this function yet or can we wait until bug 246720 is fixed?

My bug 382187 patch uses the checkForMiddleClick() function, and I'd rather not wait for a bug that's been open for 4 years with just 4 comments on it.

I hope asrail can fix all your other comments, esp. as I don't think I understand what you say on the .addTab() stuff.
(In reply to comment #39)
> (From update of attachment 341662 [details] [diff] [review])
> > function openTopWin( url, opener )
> So, I've no idea why I never noticed this before, but this function is actually
> used by View Image / Show Only This Frame code and so not only does it not
> actually open a UI link but the opener is important for security reasons.

I will leave it as it is for now, since Kairo wants the new functions in.

> >+  if (defaultAboutState == kNewWindow)
> >+    target = "window";
> >+  else if (defaultAboutState == kExistingWindow)
> >+    target = "current";
> >   else {
> >+    target = "tab";
> >   }
> Not that I like it as an if/else if/else, but the {}s are wrong. (Maybe a
> switch or an array index e.g. ["current", "window", "tab"][defaultAboutState -
> 1])

switch has better performance than the hash (array).

> >+     * XXX: we should use node.oncommand(event) once bug 246720 is fixed.
> Do we need this function yet or can we wait until bug 246720 is fixed?

Kairo answered.

> >+  if ("tagName" in node) {
> I'm not sure why this was written with recursion rather than a loop, or why it
> bothers with a tag name check rather than just null-checking...
> 
> >+    if (node.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> >+    && (node.tagName == "menupopup" || node.tagName == "popup"))
> node instanceof nsIDOMXULPopupElement

OK.

> >+ * The logic for modifiers is as fiollowing:
> Unfortunately the function then fails to follow the fiollowing (sic) logic...

What still wrong here?

> >+    browser.addTab(url, null, null, !loadInBackground, flags);
> The focus new tab parameter also links in to the restore previous tab function,
> is that intentional here? Normally we only use it for open link in new tab, and
> if we want to focus the new tab we set it as the current tab afterwards.

Kairo, it will focus the previous current tab while closing the new opened one without moving around, instead of the tab at the left of the closed tab.

> >+      browser.addTab(urlArray[i], null, null, !loadInBackground, flags);
> Similar problem here, except it makes even less sense for tab groups, where we
> would focus the first tab if at all. See OpenURLArrayInTabs.

I made it focus the first one, if not loading in background. I think it makes sense.
I couldn't think of a fair use to "current" for groups of tabs. I'm adding new tabs.
Assignee: kairo → asrail
Attachment #341662 - Attachment is obsolete: true
Attachment #341850 - Flags: review?(neil)
Comment on attachment 341850 [details] [diff] [review]
Addressing comments and fixing some issues

>+  switch (defaultAboutState){
Nit: space between ) and {

>+  case  kExistingWindow:
Nit: double space

>-  checkForUpdates.accessKey = gUtilityBundle.getString("updatesItem_" + key + "AccessKey"); 
>+  checkForUpdates.accessKey = gUtilityBundle.getString("updatesItem_" + key + "AccessKey");
>- */ 
>+ */
Nit: unrelated whitespace changes

>+    if (node && (node instanceof nsIDOMXULPopupElement))
Nit: instanceof is null-safe

I misled you slightly before; you need to use Components.interfaces.nsIDOMXULPopupElement (or defined a local const for it).

>+  var alt  =  e.altKey && !ignoreSave;
Can't ignoreSave yet, you don't know which key is the save key.

>+  // ignoreButton allows "middle-click paste" to use function without always opening in a new window.
>+  var middle = !ignoreButton && e.button == 1;
>+  var middleUsesTabs = getBoolPref("browser.tabs.opentabfor.middleclick", true);
>+
>+  // Don't do anything special with right-mouse clicks.  They're probably clicks on context menu items.
I don't see any code to handle this?

>+  if (meta || ctrl || (middle && middleUsesTabs)) {
middleUsesTabs applies to all of these modifiers, not just middle click.

>+  else if (alt) {
>+    return "save";
Not true, save key depends on a pref.

>+  else if (shift || (middle && !middleUsesTabs)) {
>+    return "window";
Shift on its own never opens a window.

>+  else {
>+    return "current";
There are some combinations that do nothing.

Nit: Don't use else if the "then" portion ends with a return.

>+    if(!loadInBackground)
Nit: (twice) space between if and (
Attachment #341850 - Flags: review?(neil) → review-
Comment on attachment 341850 [details] [diff] [review]
Addressing comments and fixing some issues

>diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
>--- a/suite/common/utilityOverlay.js
>+++ b/suite/common/utilityOverlay.js
>+function openUILinkArrayIn(urlArray, where, allowThirdPartyFixup)
>+{
...
>+  case "current":
>+  case "tab":

I object as the function should provide both the possibility to replace the current page (that's what "current" is for) and add the rest as new tabs, or add all as new tabs ("tab" option). What we use from where is a different discussion, but the function should make both ways of opening available to users, that's what matches openUILinkIn() and that's what we should offer here.
Attached patch Updating (obsolete) — Splinter Review
(In reply to comment #44)
> >+  var alt  =  e.altKey && !ignoreSave;
> Can't ignoreSave yet, you don't know which key is the save key.
> 
> >+  // ignoreButton allows "middle-click paste" to use function without always opening in a new window.
> >+  var middle = !ignoreButton && e.button == 1;
> >+  var middleUsesTabs = getBoolPref("browser.tabs.opentabfor.middleclick", true);
> >+
> >+  // Don't do anything special with right-mouse clicks.  They're probably clicks on context menu items.
> I don't see any code to handle this?

I've removed the comment. There is no difference from left and right buttons on the code.

> >+  if (meta || ctrl || (middle && middleUsesTabs)) {
> middleUsesTabs applies to all of these modifiers, not just middle click.

OK, done.
 
> >+  else if (alt) {
> >+    return "save";
> Not true, save key depends on a pref.

Got it.

> >+  else if (shift || (middle && !middleUsesTabs)) {
> >+    return "window";
> Shift on its own never opens a window.

Using the right pref here too.

> >+  else {
> >+    return "current";
> There are some combinations that do nothing.

I'm following the comments on the start of the function. It returns nothing if Ctrl, or Meta, or middle-click is pressed and both browser.tabs.opentabfor.middleclick and middlemouse.openNewWindow are false.


Nits addressed.
Attachment #341850 - Attachment is obsolete: true
Attachment #341870 - Flags: review?(neil)
Attached patch Minor fix (obsolete) — Splinter Review
I'd forgotten to fix a little issue while adding Kairo's code again for opening groups of tabs as current.
Attachment #341870 - Attachment is obsolete: true
Attachment #341910 - Flags: review?(neil)
Attachment #341870 - Flags: review?(neil)
It looks like my js mode (js2-mode) for emacs remove that space at the end of line without interaction.
Sorry for this.
Attachment #341910 - Attachment is obsolete: true
Attachment #341918 - Flags: review?(neil)
Attachment #341910 - Flags: review?(neil)
Comment on attachment 341918 [details] [diff] [review]
Undoing a space removal on the end of line

So near, yet somehow still so far ;-)

>+        node.hidePopup();
Nit: wrong indent

>+  } while((node = node.parentNode));
Nit: space between while and (
I'm not exactly keen on ((node = ...)) [to avoid the JS strict warning], maybe rewriting this loop as for (; node; node = node.parentNode) would look better.

>+  if (!e)
>+    e = { shiftKey:false, ctrlKey:false, metaKey:false, altKey:false, button:0 };
Is it me or could we just return "current" straight away?

>+  var saveKey = getBoolPref("ui.key.saveLink.shift", true) ? shift : alt;
>+  saveKey = saveKey && !ignoreSave;
>+
>+  // ignoreButton allows "middle-click paste" to use function without always opening in a new window.
>+  var middle = !ignoreButton && e.button == 1;
>+  var middleUsesTabs = getBoolPref("browser.tabs.opentabfor.middleclick", true);
>+  var openNewWindow = getBoolPref("middlemouse.openNewWindow", true);
We get all these prefs although strictly speaking we never use them all. Maybe it would be better to get them when you actually need to.

>+    return;
>+  }
>+  else if (saveKey) {
This is wrong; if both middle-click prefs are off then we should still check for the save key.

>+    return "save";
>+  }
>+  return "current";
Still need to return (one of null or "" - a lone return; is a JS strict warning) if the alt key is down (unless it was detected as the save key).

>+    for (var i = 1; i < urlArray.length; i++)
>+      browser.addTab(urlArray[i], null, null, false, flags);
>+    break;
...
>+    for (var i = 1; i < urlArray.length; i++)
>+      browser.addTab(urlArray[i], null, null, false, flags);
>+    break;
>+  }
This code could be moved to after the switch.
Attachment #341918 - Flags: review?(neil) → review-
Attached patch Addressing last comments (obsolete) — Splinter Review
(In reply to comment #49)
> >+        node.hidePopup();
> Nit: wrong indent

OK.

> >+  } while((node = node.parentNode));
> Nit: space between while and (
> I'm not exactly keen on ((node = ...)) [to avoid the JS strict warning], maybe
> rewriting this loop as for (; node; node = node.parentNode) would look better.

Rewritten as a for.

> >+  if (!e)
> >+    e = { shiftKey:false, ctrlKey:false, metaKey:false, altKey:false, button:0 };
> Is it me or could we just return "current" straight away?

Yes, we can.

> We get all these prefs although strictly speaking we never use them all. Maybe
> it would be better to get them when you actually need to.

I'm using the prefs on demand now.

> >+    return;
> >+  }
> >+  else if (saveKey) {
> This is wrong; if both middle-click prefs are off then we should still check
> for the save key.

Done. Except for middle click, where it's returning null straight away. I've compared to contantAreaClick.js and it does not check for save key when the middle button is pressed.
I had to make a little change to the comment on the start of the function due this behavior.

> >+    return "save";
> >+  }
> >+  return "current";
> Still need to return (one of null or "" - a lone return; is a JS strict
> warning) if the alt key is down (unless it was detected as the save key).

OK.

> >+    for (var i = 1; i < urlArray.length; i++)
> >+      browser.addTab(urlArray[i], null, null, false, flags);
> >+    break;
> ...
> >+    for (var i = 1; i < urlArray.length; i++)
> >+      browser.addTab(urlArray[i], null, null, false, flags);
> >+    break;
> >+  }
> This code could be moved to after the switch.

Done.
Attachment #341918 - Attachment is obsolete: true
Attachment #342264 - Flags: review?(neil)
Attached patch Update (obsolete) — Splinter Review
Very minor fix for bug reintroduced on the last patch.
Attachment #342264 - Attachment is obsolete: true
Attachment #342272 - Flags: review?(neil)
Attachment #342264 - Flags: review?(neil)
Attachment #342272 - Flags: review?(neil) → review+
Comment on attachment 342272 [details] [diff] [review]
Update

>+  var saveKey = getBoolPref("ui.key.saveLink.shift", true) ? shift : alt;
>+  saveKey = saveKey && !ignoreSave;
>+  if (saveKey) {
>+    return "save";
>+  }
Could consider wrapping this inside an if (!ignoreSave) block to avoid getting the pref.

>+  else if (alt || meta || ctrl) {
Nit: Don't need the else or the {}s here
I'm not convinced by the || meta || ctrl otherwise there would be no need to special-case middle above.

>+function openUILinkIn(url, where, allowThirdPartyFixup)
>+{
>+  if (!where || !url)
>+    return null;
>+
>+  if (where == "save") {
>+    saveURL(url, null, null, true);
>+    return null;
These were supposed to be blank returns, as they're not supposed to return anything, while whereToOpenLink is supposed to return something.
Attached patch Addressing last comments (obsolete) — Splinter Review
(In reply to comment #52)
> (From update of attachment 342272 [details] [diff] [review])
> >+  var saveKey = getBoolPref("ui.key.saveLink.shift", true) ? shift : alt;
> >+  saveKey = saveKey && !ignoreSave;
> >+  if (saveKey) {
> >+    return "save";
> >+  }
> Could consider wrapping this inside an if (!ignoreSave) block to avoid getting
> the pref.

Done. Re-asking for review due this and requesting sr now.

> >+  else if (alt || meta || ctrl) {
> Nit: Don't need the else or the {}s here

Removed. Also removed "{}" from another place with just one statement.

> I'm not convinced by the || meta || ctrl otherwise there would be no need to
> special-case middle above.

The middle special case is to make it conform with the contentAreaClick.js.
It checks first for the button... if it's middle click, it doesn't even try to save it. If it's left click, meta or control is pressed, and both prefs are false, it will check for the saveKey.
The middle special case is to make it doesn't return "save" if both middle and saveKey are pressed.
The "|| meta || ctrl" is to make it follows the current behavior of the cited file. SM won't load the links if both prefs are false and meta or control is pressed.

> >+function openUILinkIn(url, where, allowThirdPartyFixup)
> >+{
> >+  if (!where || !url)
> >+    return null;
> >+
> >+  if (where == "save") {
> >+    saveURL(url, null, null, true);
> >+    return null;
> These were supposed to be blank returns, as they're not supposed to return
> anything, while whereToOpenLink is supposed to return something.

openUILinkIn returns the window opened or the current window on the other cases.
So I though I should return something there too.
Removing, anyway.
Attachment #342272 - Attachment is obsolete: true
Attachment #342310 - Flags: superreview?(neil)
Attachment #342310 - Flags: review?(neil)
Attached patch Another minor fix (obsolete) — Splinter Review
+  switch (where) {               
+  case "current":                         
+    w.loadURI(urlArray[0], null, flags);
+    browser = w.getBrowser();                  
+  break;                            
+  case "tabshifted":

This break was missing since I moved the for.
Attachment #342310 - Attachment is obsolete: true
Attachment #342312 - Flags: superreview?(neil)
Attachment #342312 - Flags: review?(neil)
Attachment #342310 - Flags: superreview?(neil)
Attachment #342310 - Flags: review?(neil)
(In reply to comment #53)
> The "|| meta || ctrl" is to make it follows the current behavior of the cited
> file. SM won't load the links if both prefs are false and meta or control is
> pressed.
I must be overlooking where that happens... can you point me to a line of code?

> > >+    return null;
> > These were supposed to be blank returns, as they're not supposed to return
> > anything, while whereToOpenLink is supposed to return something.
> openUILinkIn returns the window opened or the current window on the other
> cases.
D'oh, I had completely overlooked that. You can put them back ;-)
Comment on attachment 342312 [details] [diff] [review]
Another minor fix

>+  if (alt || meta || ctrl)
>+    return null;
OK, so I found that the return value of handleLinkClick is ignored, so I went off and located the right code, and found out that you need to include shift here, in case the save key is alt.

>+  if (!where || !url)
>+    return;
>+
>+  if (where == "save") {
>+    saveURL(url, null, null, true);
>+    return;
>+  }
Also restore these nulls to our previous correct version and you can have r+sr=me
Attachment #342312 - Flags: superreview?(neil)
Attachment #342312 - Flags: superreview+
Attachment #342312 - Flags: review?(neil)
Attachment #342312 - Flags: review+
(In reply to comment #56)
> (From update of attachment 342312 [details] [diff] [review])
> >+  if (alt || meta || ctrl)
> >+    return null;
> OK, so I found that the return value of handleLinkClick is ignored, so I went
> off and located the right code, and found out that you need to include shift
> here, in case the save key is alt.

I were replying your comment when I saw this.
I've added shift here, I hadn't noticed.

> >+  if (!where || !url)
> >+    return;
> >+
> >+  if (where == "save") {
> >+    saveURL(url, null, null, true);
> >+    return;
> >+  }
> Also restore these nulls to our previous correct version and you can have
> r+sr=me

Restored and carrying flags.
Attachment #342312 - Attachment is obsolete: true
Attachment #342346 - Flags: superreview+
Attachment #342346 - Flags: review+
Keywords: checkin-needed
Lame typo fix:

(In reply to comment #56)
> Also restore these nulls to our previous correct version
your
Comment on attachment 342346 [details] [diff] [review]
Addressing last comments
[Checkin: Comment 59]

http://hg.mozilla.org/comm-central/rev/b9559bf7af5e
Attachment #342346 - Attachment description: Addressing last comments → Addressing last comments [Checkin: Comment 59]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a2
You need to log in before you can comment on or make changes to this bug.