Closed
Bug 583997
Opened 14 years ago
Closed 14 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)
SeaMonkey
Tabbed Browser
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a3
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(2 files, 3 obsolete files)
4.44 KB,
patch
|
misak.bugzilla
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
1.65 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | 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 1•14 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•14 years ago
|
||
(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•14 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•14 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 ?
Comment 5•14 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•14 years ago
|
||
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•14 years ago
|
Attachment #463817 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 7•14 years ago
|
||
Pushed: http://hg.mozilla.org/comm-central/rev/451fc0f384ef
Status: ASSIGNED → RESOLVED
Closed: 14 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?
Assignee | ||
Comment 9•14 years ago
|
||
Thanks ! I didn't found such easy solution :)
Attachment #463898 -
Flags: superreview?(neil)
Attachment #463898 -
Flags: review?(neil)
Assignee | ||
Comment 10•14 years ago
|
||
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 11•14 years ago
|
||
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•14 years ago
|
Target Milestone: --- → seamonkey2.1a3
Assignee | ||
Comment 12•14 years ago
|
||
Pushed with fixed nit: http://hg.mozilla.org/comm-central/rev/7c0f467d06c4
Target Milestone: seamonkey2.1a3 → ---
Updated•14 years ago
|
Target Milestone: --- → seamonkey2.1a3
You need to log in
before you can comment on or make changes to this bug.
Description
•