Closed Bug 1578067 Opened 1 year ago Closed 4 months ago

Update openLinkIn() and use more

Categories

(SeaMonkey :: General, task)

task
Not set

Tracking

(seamonkey2.49esr wontfix, seamonkey2.53 affected, seamonkey2.57esr fixed, seamonkey2.63 wontfix)

RESOLVED FIXED
seamonkey2.68
Tracking Status
seamonkey2.49esr --- wontfix
seamonkey2.53 --- affected
seamonkey2.57esr --- fixed
seamonkey2.63 --- wontfix

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 5 obsolete files)

3.41 KB, patch
frg
: review+
Details | Diff | Splinter Review
4.94 KB, patch
frg
: review+
Details | Diff | Splinter Review
1.76 KB, patch
frg
: review+
Details | Diff | Splinter Review
4.53 KB, patch
frg
: review+
Details | Diff | Splinter Review
6.13 KB, patch
frg
: review+
Details | Diff | Splinter Review
8.46 KB, patch
frg
: review+
Details | Diff | Splinter Review

Port the following:

  • Bug 606678 - use openLinkIn in nsContextMenu ("Open Link in New Tab" shouldn't add tabs to popups)
  • Bug 626148 - Opening link in new tab from background window opens tab in foreground window
  • Bug 586234 - When middle clicking links in popups, open the new tab in a full browser window
  • Bug 847952 - Make utility function to return the topmost window discriminate between public and private windows
  • Bug 549340 - reorganize ContentAreaClick, give it consistent return values and comments
  • Bug 610203 - alt+enter in location bar shouldn't open invisible tabs in popup windows (open them in a full browser window instead)
  • Bug 674161 - middle mouse paste should prevent inheriting the current page's principal
  • Bug 1038599 - Called OpenLinkIn from loadURI
    and make more use of the function openLinkIn
Attached patch Update openLinkIn (obsolete) — Splinter Review

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: None
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): None
String changes made by this patch: None

Attachment #9089708 - Flags: review?(frgrahl)
Attachment #9089708 - Flags: approval-comm-release?
Attachment #9089708 - Flags: approval-comm-esr60?
Comment on attachment 9089708 [details] [diff] [review]
Update openLinkIn

Try to open a private window via conext menu or File->New made 2.53 go into a loop opening windows. Could you retest it. Didn't find anything obvious.
Attachment #9089708 - Flags: review?(frgrahl)
Attachment #9089708 - Flags: review-
Attachment #9089708 - Flags: approval-comm-release?
Attachment #9089708 - Flags: approval-comm-esr60?
Depends on: 1579735
Attached patch Enhance openLinkIn() (obsolete) — Splinter Review
Attached patch Update contentAreaClick.js (obsolete) — Splinter Review
Attached patch Update contentAreaClick.js v1.1 (obsolete) — Splinter Review

Take account of if PB window

Attachment #9091554 - Attachment is obsolete: true
Attached patch Update contentAreaClick.js v1.1 (obsolete) — Splinter Review

Must remember to qrefresh!

Attachment #9091560 - Attachment is obsolete: true

and missed one whitespace change.

Attachment #9091561 - Attachment is obsolete: true
Comment on attachment 9091529 [details] [diff] [review]
Part 1 - Update getTopWin()

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none
Attachment #9091529 - Flags: review?(frgrahl)
Attachment #9091529 - Flags: approval-comm-release?
Attachment #9091529 - Flags: approval-comm-esr60?
Attachment #9091529 - Attachment filename: 1578067_get_top_win.patch → 1578067_part1_get_top_win.patch
Attachment #9091529 - Attachment description: Update getTopWin() → Part 1 - Update getTopWin()

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none

Attachment #9091548 - Attachment is obsolete: true
Attachment #9091936 - Flags: review?(frgrahl)
Attachment #9091936 - Flags: approval-comm-release?
Attachment #9091936 - Flags: approval-comm-esr60?
Attachment #9091936 - Attachment filename: 1578067_enhance_open_link_in_v1_1.patch → 1578067_part2_enhance_open_link_in_v1_1.patch
Comment on attachment 9091562 [details] [diff] [review]
Part 3 - Update contentAreaClick.js v1.2

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none
Attachment #9091562 - Attachment description: Update contentAreaClick.js v1.2 → Part 3 - Update contentAreaClick.js v1.2
Attachment #9091562 - Attachment filename: 1578067_update_content_area_click_v1_2.patch → 1578067_part3_update_content_area_click_v1_2.patch
Attachment #9091562 - Flags: review?(frgrahl)
Attachment #9091562 - Flags: approval-comm-release?
Attachment #9091562 - Flags: approval-comm-esr60?
Comment on attachment 9091566 [details] [diff] [review]
Part 4 - Switch loadURI() to use openLinkIn()

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none
Attachment #9091566 - Attachment description: Switch loadURI() to use openLinkIn() → Part 4 - Switch loadURI() to use openLinkIn()
Attachment #9091566 - Attachment filename: 1578067_switch_loadURI.patch → 1578067_part4_switch_loadURI.patch
Attachment #9091566 - Flags: review?(frgrahl)
Attachment #9091566 - Flags: approval-comm-release?
Attachment #9091566 - Flags: approval-comm-esr60?
Comment on attachment 9091568 [details] [diff] [review]
Part 5 - Switch context to openLinkIn()

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none
Attachment #9091568 - Attachment description: Switch context to openLinkIn() → Part 5 - Switch context to openLinkIn()
Attachment #9091568 - Attachment filename: 1578067_switch_context_to_openLinkIn.patch → 1578067_part5_switch_context_to_openLinkIn.patch
Attachment #9091568 - Flags: review?(frgrahl)
Attachment #9091568 - Flags: approval-comm-release?
Attachment #9091568 - Flags: approval-comm-esr60?

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none

Attachment #9089708 - Attachment is obsolete: true
Attachment #9091940 - Flags: review?(frgrahl)
Attachment #9091940 - Flags: approval-comm-release?
Attachment #9091940 - Flags: approval-comm-esr60?
Attachment #9091940 - Attachment filename: 1578067_switch_open_with.patch → 1578067_part6_switch_open_with.patch
Blocks: 1580348
Comment on attachment 9091529 [details] [diff] [review]
Part 1 - Update getTopWin()

LGTM

Tested with popups on: 
https://www.catalog.update.microsoft.com/Search.aspx?q=2019-10

2.49.5 opens a new tab in the popup. 2.53.1 in the first main window.
Attachment #9091529 - Flags: review?(frgrahl)
Attachment #9091529 - Flags: review+
Attachment #9091529 - Flags: approval-comm-release?
Attachment #9091529 - Flags: approval-comm-release+
Attachment #9091529 - Flags: approval-comm-esr60?
Attachment #9091529 - Flags: approval-comm-esr60+
Comment on attachment 9091562 [details] [diff] [review]
Part 3 - Update contentAreaClick.js v1.2

LGTM
Attachment #9091562 - Flags: review?(frgrahl)
Attachment #9091562 - Flags: review+
Attachment #9091562 - Flags: approval-comm-release?
Attachment #9091562 - Flags: approval-comm-release+
Attachment #9091562 - Flags: approval-comm-esr60?
Attachment #9091562 - Flags: approval-comm-esr60+
Comment on attachment 9091566 [details] [diff] [review]
Part 4 - Switch loadURI() to use openLinkIn()

LGTM
Attachment #9091566 - Flags: review?(frgrahl)
Attachment #9091566 - Flags: review+
Attachment #9091566 - Flags: approval-comm-release?
Attachment #9091566 - Flags: approval-comm-release+
Attachment #9091566 - Flags: approval-comm-esr60?
Attachment #9091566 - Flags: approval-comm-esr60+
Comment on attachment 9091936 [details] [diff] [review]
Part 2 - Enhance openLinkIn() v1.1

LGTM
Attachment #9091936 - Flags: review?(frgrahl)
Attachment #9091936 - Flags: review+
Attachment #9091936 - Flags: approval-comm-release?
Attachment #9091936 - Flags: approval-comm-release+
Attachment #9091936 - Flags: approval-comm-esr60?
Attachment #9091936 - Flags: approval-comm-esr60+
Comment on attachment 9091568 [details] [diff] [review]
Part 5 - Switch context to openLinkIn()

LGTM

Should openFrame: function()  be renamed to openFrameInWindow() for consistency in a follow-up?
Attachment #9091568 - Flags: review?(frgrahl)
Attachment #9091568 - Flags: review+
Attachment #9091568 - Flags: approval-comm-release?
Attachment #9091568 - Flags: approval-comm-release+
Attachment #9091568 - Flags: approval-comm-esr60?
Attachment #9091568 - Flags: approval-comm-esr60+
Comment on attachment 9091940 [details] [diff] [review]
Part 6 - Switch open functions to use openLinkIn()

NIT: In other parts e.g. part 3 you inline the where. Do we need it as a separate var?

+  var where = aEvent && aEvent.shiftKey ? "tabshifted" : "tab";
+  return openNewTabWindowOrExistingWith(aURL, where, aDocument, null,

Do we need a noreferrer with this removed?
-  else if (aNode instanceof Element &&
[23:53]	frg_away	-           !/(?:^|\s)noreferrer(?:\s|$)/i.test(aNode.getAttribute("rel")))

See https://dxr.mozilla.org/mozilla-esr60/source/browser/modules/ContextMenu.jsm#964

r/a+ with question answered.
Attachment #9091940 - Flags: review?(frgrahl)
Attachment #9091940 - Flags: review+
Attachment #9091940 - Flags: approval-comm-release?
Attachment #9091940 - Flags: approval-comm-release+
Attachment #9091940 - Flags: approval-comm-esr60?
Attachment #9091940 - Flags: approval-comm-esr60+

Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/cf230e323194
Part 1: Update openLinkIn() and use more - getTopWin patch. r=frg
https://hg.mozilla.org/comm-central/rev/aafc1fdafb54
Part 2: Update openLinkIn() and use more - update openLinkIn(). r=frg
https://hg.mozilla.org/comm-central/rev/8f6a7dd3996f
Part 3: Update openLinkIn() and use more - update contentAreaClick. r=frg
https://hg.mozilla.org/comm-central/rev/f171994fe29a
Part 4: Update openLinkIn() and use more - switch loadURI. r=frg
https://hg.mozilla.org/comm-central/rev/87497ed899d2
Part 5: Update openLinkIn() and use more - Switch context menus to use openLinkIn(). r=frg
https://hg.mozilla.org/comm-central/rev/e8fe8731e1ae
Part 6: Update openLinkIn() and use more - switch open functions to openLinkIn(). r=frg

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

https://hg.mozilla.org/releases/comm-esr60/rev/87753e79a0c07af2493d356763c96c28b2a7944f
Bug 1578067 - Part 1: Update openLinkIn() and use more - getTopWin patch. r=frg a=frg
https://hg.mozilla.org/releases/comm-esr60/rev/0296e6ac36793ab1e62898e0afdbc5ad178d27e2
Bug 1578067 - Part 2: Update openLinkIn() and use more - update openLinkIn(). r=frg a=frg
https://hg.mozilla.org/releases/comm-esr60/rev/ebb0ef5a307c7b14794a247f2b867788f6dd301a
Bug 1578067 - Part 3: Update openLinkIn() and use more - update contentAreaClick. r=frg a=frg
https://hg.mozilla.org/releases/comm-esr60/rev/0c7810b7c5c04393f3dd2343ef4f896fd9943d0b
Bug 1578067 - Part 4: Update openLinkIn() and use more - switch loadURI. r=frg a=frg
https://hg.mozilla.org/releases/comm-esr60/rev/3665fe3f7aaa9210d696bb516c85054949e15442
Bug 1578067 - Part 5: Update openLinkIn() and use more - Switch context menus to use openLinkIn(). r=frg a=frg
https://hg.mozilla.org/releases/comm-esr60/rev/a1d8a917f458bc7be7b59779323b89979f886e88
Bug 1578067 - Part 6: Update openLinkIn() and use more - switch open functions to openLinkIn(). r=frg a=frg

Target Milestone: --- → seamonkey2.68
You need to log in before you can comment on or make changes to this bug.