Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1984573002:
Prevent chrome.windows.create from moving a tab to another profile (Closed)
|
Created:
4 years, 7 months ago by robwu Modified:
4 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent chrome.windows.create from moving a tab to another profile
BUG=609942
TEST=browser_tests --gtest_filter=ExtensionApiTest.Incognito
Committed: https://crrev.com/dbe4c7255f2c8ce9307aa20a73e4d293e9f60a2f
Cr-Commit-Position: refs/heads/master@{#394513}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Devlin's nits #
Messages
Total messages: 18 (7 generated)
rob@robwu.nl changed reviewers: + finnur@chromium.org
finnur@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
LGTM, one nit, but I would feel better if Devlin (or someone) gave OWNERS permission on this one. https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:467: return false; // error_ member is set inside of ShouldOpenIncognitoWindow. nit: This comment doesn't really add anything, I wouldn't retain it.
https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:467: return false; // error_ member is set inside of ShouldOpenIncognitoWindow. On 2016/05/17 14:49:14, Finnur wrote: > nit: This comment doesn't really add anything, I wouldn't retain it. I disagree a little - it explains why we don't have to set the error_ message separately here (not to be mistaken with is_error). If I'm just looking at this code, I should be on the look out for returning false without having set error_ appropriately. https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:473: Browser* source_browser = nullptr; nit: source_browser, to me, implies that this is the browser that sent the message. Maybe "tabs_old_browser"? "move_tab_from_browser"? Something along those lines? https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:491: Profile::INCOGNITO_PROFILE; nit: I think we typically use BrowserContext::IsOffTheRecord() for this check (so here, just profile()->IsOffTheRecord()). Is there a reason to prefer GetProfileType()? https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:493: error_ = keys::kCanOnlyMoveTabsWithinSameProfileError; I also wonder if there's some other tricky edge case we might potentially want to avoid in the future (I can't think of any yet, but the types of profiles we have are growing). Maybe we could just do: if (window_profile != tabs_profile) { // Abort } ? It seems like that's really what we want to avoid, so it'd make sense to make that the direct check. (That would mean moving window_profile around, but I think that's fine)
https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:473: Browser* source_browser = nullptr; On 2016/05/17 19:29:54, Devlin wrote: > nit: source_browser, to me, implies that this is the browser that sent the > message. Maybe "tabs_old_browser"? "move_tab_from_browser"? Something along > those lines? source as in source of the tab. I'm using source_* to be consistent with "source_tab_strip". And also with MoveTab. https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:493: error_ = keys::kCanOnlyMoveTabsWithinSameProfileError; On 2016/05/17 19:29:54, Devlin wrote: > I also wonder if there's some other tricky edge case we might potentially want > to avoid in the future (I can't think of any yet, but the types of profiles we > have are growing). Maybe we could just do: > if (window_profile != tabs_profile) { > // Abort > } > ? > > It seems like that's really what we want to avoid, so it'd make sense to make > that the direct check. > > (That would mean moving window_profile around, but I think that's fine) Done.
https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:467: return false; // error_ member is set inside of ShouldOpenIncognitoWindow. Oh, error_member! Sorry, I read this as: // is_error is set inside of ShouldOpenIncognitoWindow. My bad. Fine to keep it.
lgtm https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1984573002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:473: Browser* source_browser = nullptr; On 2016/05/17 21:14:24, robwu wrote: > On 2016/05/17 19:29:54, Devlin wrote: > > nit: source_browser, to me, implies that this is the browser that sent the > > message. Maybe "tabs_old_browser"? "move_tab_from_browser"? Something along > > those lines? > > source as in source of the tab. I'm using source_* to be consistent with > "source_tab_strip". And also with MoveTab. I still disagree with the naming, but fair point regarding consistency. This is fine.
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org Link to the patchset: https://codereview.chromium.org/1984573002/#ps20001 (title: "Devlin's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984573002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984573002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984573002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984573002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Prevent chrome.windows.create from moving a tab to another profile BUG=609942 TEST=browser_tests --gtest_filter=ExtensionApiTest.Incognito ========== to ========== Prevent chrome.windows.create from moving a tab to another profile BUG=609942 TEST=browser_tests --gtest_filter=ExtensionApiTest.Incognito Committed: https://crrev.com/dbe4c7255f2c8ce9307aa20a73e4d293e9f60a2f Cr-Commit-Position: refs/heads/master@{#394513} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dbe4c7255f2c8ce9307aa20a73e4d293e9f60a2f Cr-Commit-Position: refs/heads/master@{#394513} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
