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

RESOLVED FIXED in seamonkey2.1a3

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

unspecified
seamonkey2.1a3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

4.44 KB, patch
Misak Khachatryan
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
1.65 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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.
Attachment #462349 - Flags: review?(neil)

Comment 1

7 years ago
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+
(Assignee)

Comment 2

7 years ago
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.
Attachment #462349 - Attachment is obsolete: true
Attachment #462358 - Flags: superreview?(neil)
Attachment #462358 - Flags: review+

Comment 3

7 years ago
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-
(Assignee)

Comment 4

7 years ago
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 ?

Updated

7 years ago
Blocks: 583567

Comment 5

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

Comment 6

7 years ago
Created attachment 463817 [details] [diff] [review]
for checkin, fixed

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+

Updated

7 years ago
Attachment #463817 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 7

7 years ago
Pushed: http://hg.mozilla.org/comm-central/rev/451fc0f384ef
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 8

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

Comment 9

7 years ago
Created attachment 463898 [details] [diff] [review]
optimize openUILinkIn logic

Thanks ! I didn't found such easy solution :)
Attachment #463898 - Flags: superreview?(neil)
Attachment #463898 - Flags: review?(neil)
(Assignee)

Comment 10

7 years ago
Created attachment 463899 [details] [diff] [review]
optimize openUILinkIn logic, actual patch

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+

Updated

7 years ago
Target Milestone: --- → seamonkey2.1a3
(Assignee)

Comment 12

7 years ago
Pushed with fixed nit: http://hg.mozilla.org/comm-central/rev/7c0f467d06c4
Target Milestone: seamonkey2.1a3 → ---

Updated

7 years ago
Target Milestone: --- → seamonkey2.1a3
You need to log in before you can comment on or make changes to this bug.