Last Comment Bug 583997 - Fix parameter usage in loadOneTab and addTab, also add new option to openUILinkIn to explicitly focus newly opened tab
: Fix parameter usage in loadOneTab and addTab, also add new option to openUILi...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: unspecified
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Misak Khachatryan
:
:
Mentors:
Depends on: 558614 566593
Blocks: 583567
  Show dependency treegraph
 
Reported: 2010-08-03 02:09 PDT by Misak Khachatryan
Modified: 2010-08-09 06:37 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (5.77 KB, patch)
2010-08-03 02:09 PDT, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
for checkin (6.13 KB, patch)
2010-08-03 04:18 PDT, Misak Khachatryan
misak.bugzilla: review+
neil: superreview-
Details | Diff | Splinter Review
for checkin, fixed (4.44 KB, patch)
2010-08-07 10:34 PDT, Misak Khachatryan
misak.bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review
optimize openUILinkIn logic (71 bytes, patch)
2010-08-08 03:20 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review
optimize openUILinkIn logic, actual patch (1.65 KB, patch)
2010-08-08 03:24 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Misak Khachatryan 2010-08-03 02:09:19 PDT
Created attachment 462349 [details] [diff] [review]
fix

Recently i noticed warning on my error console related to various parameters not defined in loadOneTab and addTab. Also even after landing of bug 566593 Addon Manager opens in background for me. It seems that in review process between attachments https://bugzilla.mozilla.org/attachment.cgi?id=446964 and https://bugzilla.mozilla.org/attachment.cgi?id=451524 in bug 558614 fixing process i somehow dropped parameter parsing code.

I introduced new option "tabfocused" for where parameter in function openUILinkIn to explicitly tell to focus newly opened tab and used it in switchToTabHavingURI to fix Addon Manager.
Comment 1 neil@parkwaycc.co.uk 2010-08-03 02:40:03 PDT
Comment on attachment 462349 [details] [diff] [review]
fix

>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
These changes make no sense.

The rest of the patch is OK but it would be nice if goAbout used tabfocused.

[I don't expect you to find the sneaky way of avoiding code duplication]
Comment 2 Misak Khachatryan 2010-08-03 04:18:01 PDT
Created attachment 462358 [details] [diff] [review]
for checkin

(In reply to comment #1)
> >diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
> These changes make no sense.
> 
I'll prefer to keep changes i make for easier porting in the future, if You don't mind.

> The rest of the patch is OK but it would be nice if goAbout used tabfocused.
> 
Done.

> [I don't expect you to find the sneaky way of avoiding code duplication]
I tried to avoid code duplication, but then decided that it's better for now to leave it, until i became more experienced.
Comment 3 neil@parkwaycc.co.uk 2010-08-03 05:47:50 PDT
Comment on attachment 462358 [details] [diff] [review]
for checkin

>+            var aRelatedToCurrent;
>+            if (arguments.length == 2 &&
>+                arguments[1] != null &&
>+                typeof arguments[1] == "object" &&
>+                !(arguments[1] instanceof Components.interfaces.nsIURI)) {
>+              var params = arguments[1];
>+              aReferrerURI          = params.referrerURI;
>+              aCharset              = params.charset;
>+              aPostData             = params.postData;
>+              aLoadInBackground     = params.inBackground;
>+              aAllowThirdPartyFixup = params.allowThirdPartyFixup;
>+              aRelatedToCurrent     = params.relatedToCurrent;
This change makes no sense because we don't have all these variables. Instead we just pass params directly to addTab.

>-              ({
>-                referrerURI: aReferrerURI,
>-                charset: aCharset,
>-                postData: aPostData,
>-                focusNewTab: aFocusNewTab,
>-                allowThirdPartyFixup: aAllowThirdPartyFixup,
>-                relatedToCurrent: aRelatedToCurrent
>-              } = arguments[1]);
>+              let params = arguments[1];
>+              aReferrerURI          = params.referrerURI;
>+              aCharset              = params.charset;
>+              aPostData             = params.postData;
>+              aOwner                = params.ownerTab;
>+              aAllowThirdPartyFixup = params.allowThirdPartyFixup;
>+              aRelatedToCurrent     = params.relatedToCurrent;
>+              aFocusNewtab          = params.focusNewTab;
The owner change is definitely wrong. The other changes aren't necessary, and are also in the wrong order anyway, so I wouldn't bother with them either.
Comment 4 Misak Khachatryan 2010-08-03 21:47:32 PDT
I'm getting these warnings on console without changes in tabbrowser.xml, when opening Addon Manager:

Warning: reference to undefined property "charset"
Source file: chrome://navigator/content/tabbrowser.xml
Line: 1455
Warning: reference to undefined property "focusNewTab"
Source file: chrome://navigator/content/tabbrowser.xml
Line: 1455

And this warining, when middle clicking on link:
Warning: reference to undefined property "relatedToCurrent"
Source file: chrome://navigator/content/tabbrowser.xml
Line: 1455

How i can fix them ?
Comment 5 neil@parkwaycc.co.uk 2010-08-07 04:16:29 PDT
(In reply to comment #3)
> >+              let params = arguments[1];
> >+              aReferrerURI          = params.referrerURI;
> >+              aCharset              = params.charset;
> >+              aPostData             = params.postData;
> >+              aOwner                = params.ownerTab;
> >+              aAllowThirdPartyFixup = params.allowThirdPartyFixup;
> >+              aRelatedToCurrent     = params.relatedToCurrent;
> >+              aFocusNewtab          = params.focusNewTab;
> The owner change is definitely wrong. The other changes aren't necessary, and
> are also in the wrong order anyway, so I wouldn't bother with them either.
Sigh. I didn't realise the JS engine was so inconsistent. This is alright as long as you fix it properly, using the correct variables.
Comment 6 Misak Khachatryan 2010-08-07 10:34:26 PDT
Created attachment 463817 [details] [diff] [review]
for checkin, fixed

I removed parameter code from loadOneTab and fixed it for addTab.
Comment 7 Misak Khachatryan 2010-08-07 12:19:48 PDT
Pushed: http://hg.mozilla.org/comm-central/rev/451fc0f384ef
Comment 8 Ian Neal 2010-08-07 15:39:20 PDT
Comment on attachment 463817 [details] [diff] [review]
for checkin, fixed

>diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
case "tabfocused":
  // forces tab to be focused
  loadInBackground = true;
  // fall through
case "tabshifted":
  loadInBackground = !loadInBackground;
  // fall through
case "tab":
  var browser = w.getBrowser();
  var tab = browser.addTab(url, {
              referrerURI: aReferrerURI,
              postData: aPostData,
              allowThirdPartyFixup: aAllowThirdPartyFixup,
              relatedToCurrent: aRelatedToCurrent
            });
  if (!loadInBackground) {
    browser.selectedTab = tab;
    w.content.focus();
  }

Wouldn't the above have been better to avoid code duplication?
Comment 9 Misak Khachatryan 2010-08-08 03:20:20 PDT
Created attachment 463898 [details] [diff] [review]
optimize openUILinkIn logic

Thanks ! I didn't found such easy solution :)
Comment 10 Misak Khachatryan 2010-08-08 03:24:57 PDT
Created attachment 463899 [details] [diff] [review]
optimize openUILinkIn logic, actual patch

Sorry, forgot to qrefresh :(
Comment 11 neil@parkwaycc.co.uk 2010-08-08 12:14:21 PDT
Comment on attachment 463899 [details] [diff] [review]
optimize openUILinkIn logic, actual patch

>-    break;
>-  case "tabfocused":
Nit: you didn't add this break, so no need to remove it.
Comment 12 Misak Khachatryan 2010-08-09 05:55:11 PDT
Pushed with fixed nit: http://hg.mozilla.org/comm-central/rev/7c0f467d06c4

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