Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676393002:
Extensions: Only display host name for the overridden home and start-up pages. (Closed)
|
Created:
3 years, 10 months ago by karandeepb Modified:
3 years, 10 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensions: Only display host name for the overridden home and start-up pages.
Currently, the full path is shown for the home and start-up settings override
permissions. This makes the dialog UI look badly formatted and
incomprehensible. This CL changes the dialogs to only show the host name for
the overridden home and start-up pages. Existing tests are also modified.
BUG=670890
TEST=On Mac(trunk only) or Windows, open
https://chrome.google.com/webstore/detail/msn-homepage/ibflkkanbidceofpmolhpijgminhbmnm.
Click on Add to Chrome. Ensure only the host name is shown (without the www
prefix) for the new home and start-up page, on the dialog which shows up.
Review-Url: https://codereview.chromium.org/2676393002
Cr-Commit-Position: refs/heads/master@{#448805}
Committed: https://chromium.googlesource.com/chromium/src/+/0fdd4984f81bebc66dc2c36e4336f60328802ff9
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review #
Messages
Total messages: 23 (13 generated)
The CQ bit was checked by karandeepb@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.
Description was changed from ========== Extensions: Format msg. ========== to ========== Extensions: Only display host name for the overridden home and start-up pages. Currently, the full path is shown for the home and start-up settings override permissions. This makes the dialog UI look badly formatted and incomprehensible. This CL changes the dialogs to only show the host name for the overridden home and start-up pages. Existing tests are also modified. BUG=670890 TEST=On Mac(trunk only) or Windows, open https://chrome.google.com/webstore/detail/msn-homepage/ibflkkanbidceofpmolhpi.... Ensure only the host name is shown for the new home and start-up page. ==========
Description was changed from ========== Extensions: Only display host name for the overridden home and start-up pages. Currently, the full path is shown for the home and start-up settings override permissions. This makes the dialog UI look badly formatted and incomprehensible. This CL changes the dialogs to only show the host name for the overridden home and start-up pages. Existing tests are also modified. BUG=670890 TEST=On Mac(trunk only) or Windows, open https://chrome.google.com/webstore/detail/msn-homepage/ibflkkanbidceofpmolhpi.... Ensure only the host name is shown for the new home and start-up page. ========== to ========== Extensions: Only display host name for the overridden home and start-up pages. Currently, the full path is shown for the home and start-up settings override permissions. This makes the dialog UI look badly formatted and incomprehensible. This CL changes the dialogs to only show the host name for the overridden home and start-up pages. Existing tests are also modified. BUG=670890 TEST=On Mac(trunk only) or Windows, open https://chrome.google.com/webstore/detail/msn-homepage/ibflkkanbidceofpmolhpi.... Click on Add to Chrome. Ensure only the host name is shown (without the www prefix) for the new home and start-up page, on the dialog which shows up. ==========
karandeepb@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
PTAL Devlin. We might want to change the message shown to something like "Change your home page to be on google.com". Although this is consistent with what we have for search settings currently.
Nice! lgtm https://codereview.chromium.org/2676393002/diff/1/chrome/common/extensions/ma... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/2676393002/diff/1/chrome/common/extensions/ma... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:106: std::string RemoveWwwPrefix(const std::string& url) { Optional nit: RemoveWwwPrefix is now only used from FormatUrlForDisplay - we could just inline it. https://codereview.chromium.org/2676393002/diff/1/chrome/common/extensions/ma... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:113: return RemoveWwwPrefix(url.host()); Let's take the opportunity to make this a bit faster and use base::StringPieces here (use GURL::host_piece() instead of host(), and, if you don't inline RemoveWwwPrefix, have it take a base::StringPiece).
https://codereview.chromium.org/2676393002/diff/1/chrome/common/extensions/ma... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/2676393002/diff/1/chrome/common/extensions/ma... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:106: std::string RemoveWwwPrefix(const std::string& url) { On 2017/02/07 18:39:24, Devlin wrote: > Optional nit: RemoveWwwPrefix is now only used from FormatUrlForDisplay - we > could just inline it. Done. https://codereview.chromium.org/2676393002/diff/1/chrome/common/extensions/ma... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:113: return RemoveWwwPrefix(url.host()); On 2017/02/07 18:39:24, Devlin wrote: > Let's take the opportunity to make this a bit faster and use base::StringPieces > here (use GURL::host_piece() instead of host(), and, if you don't inline > RemoveWwwPrefix, have it take a base::StringPiece). Done.
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@ for chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc review.
sky@chromium.org changed reviewers: - sky@chromium.org
devlin is an owner of all this code. I'm removing myself from the review.
On 2017/02/07 20:42:20, sky wrote: > devlin is an owner of all this code. I'm removing myself from the review. Oh ok! The Chromite Butler chrome extension that recommends reviewers somehow did not detect this.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2676393002/#ps20001 (title: "Address review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/07 20:56:55, karandeepb wrote: > On 2017/02/07 20:42:20, sky wrote: > > devlin is an owner of all this code. I'm removing myself from the review. > > Oh ok! The Chromite Butler chrome extension that recommends reviewers somehow > did not detect this. I wonder if it doesn't understand file:// syntax? https://chromium.googlesource.com/chromium/src/+/9c327a61afee72b188f3e80f6bfb...
On 2017/02/07 21:03:13, Devlin wrote: > On 2017/02/07 20:56:55, karandeepb wrote: > > On 2017/02/07 20:42:20, sky wrote: > > > devlin is an owner of all this code. I'm removing myself from the review. > > > > Oh ok! The Chromite Butler chrome extension that recommends reviewers somehow > > did not detect this. > > I wonder if it doesn't understand file:// syntax? > https://chromium.googlesource.com/chromium/src/+/9c327a61afee72b188f3e80f6bfb... Seems to be the case. "file://extensions/OWNERS" is listed as an owner by the extension.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486501021081420,
"parent_rev": "17bb41c1fd9bf0491a4d27c470c72b11aa0886ad", "commit_rev":
"0fdd4984f81bebc66dc2c36e4336f60328802ff9"}
Message was sent while issue was closed.
Description was changed from ========== Extensions: Only display host name for the overridden home and start-up pages. Currently, the full path is shown for the home and start-up settings override permissions. This makes the dialog UI look badly formatted and incomprehensible. This CL changes the dialogs to only show the host name for the overridden home and start-up pages. Existing tests are also modified. BUG=670890 TEST=On Mac(trunk only) or Windows, open https://chrome.google.com/webstore/detail/msn-homepage/ibflkkanbidceofpmolhpi.... Click on Add to Chrome. Ensure only the host name is shown (without the www prefix) for the new home and start-up page, on the dialog which shows up. ========== to ========== Extensions: Only display host name for the overridden home and start-up pages. Currently, the full path is shown for the home and start-up settings override permissions. This makes the dialog UI look badly formatted and incomprehensible. This CL changes the dialogs to only show the host name for the overridden home and start-up pages. Existing tests are also modified. BUG=670890 TEST=On Mac(trunk only) or Windows, open https://chrome.google.com/webstore/detail/msn-homepage/ibflkkanbidceofpmolhpi.... Click on Add to Chrome. Ensure only the host name is shown (without the www prefix) for the new home and start-up page, on the dialog which shows up. Review-Url: https://codereview.chromium.org/2676393002 Cr-Commit-Position: refs/heads/master@{#448805} Committed: https://chromium.googlesource.com/chromium/src/+/0fdd4984f81bebc66dc2c36e4336... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0fdd4984f81bebc66dc2c36e4336... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
