Closed Bug 583997 Opened 9 years ago Closed 9 years ago

Fix parameter usage in loadOneTab and addTab, also add new option to openUILinkIn to explicitly focus newly opened tab

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a3

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch fix (obsolete) — Splinter Review
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.
Attachment #462349 - Flags: review?(neil)
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]
Attachment #462349 - Flags: review?(neil) → review+
Attached patch for checkin (obsolete) — Splinter Review
(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.
Attachment #462349 - Attachment is obsolete: true
Attachment #462358 - Flags: superreview?(neil)
Attachment #462358 - Flags: review+
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.
Attachment #462358 - Flags: superreview?(neil) → superreview-
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 ?
Blocks: 583567
(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.
I removed parameter code from loadOneTab and fixed it for addTab.
Attachment #462358 - Attachment is obsolete: true
Attachment #463817 - Flags: superreview?(neil)
Attachment #463817 - Flags: review+
Attachment #463817 - Flags: superreview?(neil) → superreview+
Pushed: http://hg.mozilla.org/comm-central/rev/451fc0f384ef
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?
Attached patch optimize openUILinkIn logic (obsolete) — Splinter Review
Thanks ! I didn't found such easy solution :)
Attachment #463898 - Flags: superreview?(neil)
Attachment #463898 - Flags: review?(neil)
Sorry, forgot to qrefresh :(
Attachment #463898 - Attachment is obsolete: true
Attachment #463899 - Flags: superreview?(neil)
Attachment #463899 - Flags: review?(neil)
Attachment #463898 - Flags: superreview?(neil)
Attachment #463898 - Flags: review?(neil)
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.
Attachment #463899 - Flags: superreview?(neil)
Attachment #463899 - Flags: superreview+
Attachment #463899 - Flags: review?(neil)
Attachment #463899 - Flags: review+
Target Milestone: --- → seamonkey2.1a3
Pushed with fixed nit: http://hg.mozilla.org/comm-central/rev/7c0f467d06c4
Target Milestone: seamonkey2.1a3 → ---
Target Milestone: --- → seamonkey2.1a3
You need to log in before you can comment on or make changes to this bug.