Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924943002:
MD Settings/On Startup: ignore current tab when using current pages (Closed)
|
Created:
3 years, 6 months ago by Dan Beam Modified:
3 years, 6 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, michaelpg+watch-options_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings/On Startup: ignore current tab when using current pages
This changes the approach away from a URL blacklist.
R=dschuyler@chromium.org
BUG=722623
Review-Url: https://codereview.chromium.org/2924943002
Cr-Commit-Position: refs/heads/master@{#477823}
Committed: https://chromium.googlesource.com/chromium/src/+/6f94b0755c37c196d8459119dfb933d9d5e744dc
Patch Set 1 #
Total comments: 7
Patch Set 2 : ignore_contents #
Messages
Total messages: 27 (15 generated)
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.h (right): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.h:56: // browser. |source_contents| is omitted from the open pages. Maybe: s/is/and Chrome dev tools are/
https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.h (right): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.h:56: // browser. |source_contents| is omitted from the open pages. On 2017/06/07 01:20:26, dschuyler wrote: > Maybe: s/is/and Chrome dev tools are/ ehhh, i don't want to update this every time we change things we don't consider "currently opened in the browser".
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dbeam@chromium.org changed reviewers: + sky@chromium.org
+sky@ for chrome/browser/custom_home_pages_table_model.*
https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.cc (left): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.cc:40: if (url.SchemeIs(content::kChromeUIScheme)) { How come none of the following matters anymore? https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.h (right): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.h:57: void SetToCurrentlyOpenPages(const content::WebContents* source_contents); source_contents->ignore_contents ?
https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.cc (left): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.cc:40: if (url.SchemeIs(content::kChromeUIScheme)) { On 2017/06/07 16:08:48, sky wrote: > How come none of the following matters anymore? this code was a roundabout way to ignore the WebContents that started a "Use current tabs" request. I changed the code to just pass the requesting WebContents. this is simpler, saner, and has the mild mild benefit of working more expectedly in the case of another settings page being open in your "current tabs" other than the one you click "Use current tabs" to set your Startup pages from. https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.cc (right): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.cc:171: if (!url.is_empty() && !url.SchemeIs(content::kChromeDevToolsScheme)) and the empty and devtools check is still here
https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... File chrome/browser/custom_home_pages_table_model.h (right): https://codereview.chromium.org/2924943002/diff/1/chrome/browser/custom_home_... chrome/browser/custom_home_pages_table_model.h:57: void SetToCurrentlyOpenPages(const content::WebContents* source_contents); On 2017/06/07 16:08:48, sky wrote: > source_contents->ignore_contents ? Done.
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ok, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2924943002/#ps20001 (title: "ignore_contents")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496880861866490,
"parent_rev": "586612f5cb614fadb37cb05602dba204154e645e", "commit_rev":
"6f94b0755c37c196d8459119dfb933d9d5e744dc"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings/On Startup: ignore current tab when using current pages This changes the approach away from a URL blacklist. R=dschuyler@chromium.org BUG=722623 ========== to ========== MD Settings/On Startup: ignore current tab when using current pages This changes the approach away from a URL blacklist. R=dschuyler@chromium.org BUG=722623 Review-Url: https://codereview.chromium.org/2924943002 Cr-Commit-Position: refs/heads/master@{#477823} Committed: https://chromium.googlesource.com/chromium/src/+/6f94b0755c37c196d8459119dfb9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/6f94b0755c37c196d8459119dfb9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
