Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(732)

Issue 183923025: Garden-o-matic: Introduce an option to open links in a new window (Closed)

Created:
6 years, 9 months ago by apavlov
Modified:
6 years, 9 months ago
CC:
blink-reviews, alecflett
Visibility:
Public.

Description

Garden-o-matic: Introduce an option to open links in a new window - Add a checkbox and have its setting stored in localStorage. - Update all non-hash anchor targets whenever the settings is changed. - Honor the setting in ui.createLinkNode (moved from base.createLinkNode, since it is a UI-related task, in fact). - Drive-by: fix (and simplify) tab switching so that disabled tabs will no longer get activated. R=dglazkov@chromium.org, eseidel NOTRY=true Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168439

Patch Set 1 #

Total comments: 16

Patch Set 2 : Comments addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -27 lines) Patch
M Tools/GardeningServer/scripts/base.js View 1 chunk +0 lines, -10 lines 0 comments Download
M Tools/GardeningServer/scripts/ui.js View 1 9 chunks +53 lines, -14 lines 0 comments Download
M Tools/GardeningServer/scripts/ui/failures.js View 1 chunk +1 line, -0 lines 0 comments Download
M Tools/GardeningServer/scripts/ui/notifications.js View 3 chunks +3 lines, -3 lines 0 comments Download
M Tools/GardeningServer/scripts/ui/results.js View 1 chunk +1 line, -0 lines 0 comments Download
M Tools/GardeningServer/scripts/ui_unittests.js View 1 chunk +1 line, -0 lines 0 comments Download
M Tools/GardeningServer/styles/onebar.css View 1 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
apavlov
6 years, 9 months ago (2014-03-04 16:29:51 UTC) #1
dglazkov
+ojan Do we need this as an option?
6 years, 9 months ago (2014-03-04 17:31:04 UTC) #2
eseidel
Eh. I'd just pick a behavior (like gmail does) and let power-users use command-click, etc. ...
6 years, 9 months ago (2014-03-04 17:55:42 UTC) #3
eseidel
But I don't feel strongly. This is a developer-facing product, and we can assume our ...
6 years, 9 months ago (2014-03-04 17:56:09 UTC) #4
apavlov
On 2014/03/04 17:55:42, eseidel wrote: > Eh. I'd just pick a behavior (like gmail does) ...
6 years, 9 months ago (2014-03-04 18:46:14 UTC) #5
apavlov
Sorry, forgot to post my comments for the patch. Please see below (if we choose ...
6 years, 9 months ago (2014-03-04 18:46:54 UTC) #6
ojan
lgtm GOM used to open all links in a new window. pdr changed that recently. ...
6 years, 9 months ago (2014-03-04 22:49:02 UTC) #7
pdr.
This may be better as a chrome extension but I don't think this will negatively ...
6 years, 9 months ago (2014-03-04 23:12:09 UTC) #8
eseidel
I see GOM's largest issue as lack of a clear owner to push it forward, ...
6 years, 9 months ago (2014-03-04 23:29:44 UTC) #9
apavlov
https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js File Tools/GardeningServer/scripts/ui.js (right): https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js#newcode74 Tools/GardeningServer/scripts/ui.js:74: if (!anchor.href.indexOf('#')) On 2014/03/04 23:12:09, pdr wrote: > Can ...
6 years, 9 months ago (2014-03-05 00:23:56 UTC) #10
ojan
Still lgtm https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js File Tools/GardeningServer/scripts/ui.js (right): https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js#newcode82 Tools/GardeningServer/scripts/ui.js:82: ui.setUseNewWindowForLinks = function(enabled) On 2014/03/05 00:23:56, apavlov ...
6 years, 9 months ago (2014-03-05 01:04:20 UTC) #11
ojan
On 2014/03/04 23:29:44, eseidel wrote: > I see GOM's largest issue as lack of a ...
6 years, 9 months ago (2014-03-05 01:06:04 UTC) #12
alecflett
lgtm with one minor bit https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js File Tools/GardeningServer/scripts/ui.js (right): https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js#newcode74 Tools/GardeningServer/scripts/ui.js:74: if (!anchor.href.indexOf('#')) I've found ...
6 years, 9 months ago (2014-03-05 01:21:30 UTC) #13
apavlov
https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js File Tools/GardeningServer/scripts/ui.js (right): https://codereview.chromium.org/183923025/diff/1/Tools/GardeningServer/scripts/ui.js#newcode74 Tools/GardeningServer/scripts/ui.js:74: if (!anchor.href.indexOf('#')) On 2014/03/05 01:21:30, alecflett wrote: > I've ...
6 years, 9 months ago (2014-03-05 06:56:21 UTC) #14
apavlov
The CQ bit was checked by apavlov@chromium.org
6 years, 9 months ago (2014-03-05 06:57:52 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apavlov@chromium.org/183923025/20001
6 years, 9 months ago (2014-03-05 06:58:45 UTC) #16
commit-bot: I haz the power
6 years, 9 months ago (2014-03-05 07:00:04 UTC) #17
Message was sent while issue was closed.
Change committed as 168439

Powered by Google App Engine
This is Rietveld 408576698