Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067033002:
Discarded property on chrome.tabs.Tab and chrome.tabs.query() (Closed)
|
Created:
4 years, 6 months ago by Anderson Silva Modified:
4 years, 5 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding new Discarded property on Tab type and query() function on existing Tabs API.
BUG=621070
Committed: https://crrev.com/12caff1b13a6b2b7721897240aaa143fc8714276
Cr-Commit-Position: refs/heads/master@{#405492}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 17
Patch Set 5 : #Patch Set 6 : Merged tot. #Patch Set 7 : browsertest to unittest #
Total comments: 13
Patch Set 8 : fixing #Patch Set 9 : fix comment #
Total comments: 4
Patch Set 10 : testing tab_object creation #Patch Set 11 : back to browser_test #
Total comments: 2
Patch Set 12 : added lambda expression to simplify test #
Dependent Patchsets: Messages
Total messages: 58 (16 generated)
Description was changed from ========== Discarded property on tabs.Tab and tabs.query() BUG= ========== to ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG= ==========
andersoncss@google.com changed reviewers: + chrisha@chromium.org, georgesak@chromium.org
PTAL
https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:1002: // If tab_manager is not present, none tabs are discarded and therefore ... no* tabs... https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:1004: // check the matching of the parameter and the status on tab_manager. Maybe slightly clearer as follows, less lines, and less duplication: bool discarded = false; if (tab_manager) discarded = tab_manager->IsTabDiscarded(web_contents); if (!MatchesBool(params->query_info.discarded.get(), discarded) continue; https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_tab_util.cc:395: g_browser_process ? g_browser_process->GetTabManager() : nullptr; Can g_browser_process every not exist in this context? ie: it's likely safe to simply: DCHECK(g_browser_process); memory::TabManager* tab_manager = g_browser_process->GetTabManager();
I'll wait for Chris' comments to be addressed before reviewing this in more details. Ideally, some test coverage would be good as well. https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_tab_util.cc:395: g_browser_process ? g_browser_process->GetTabManager() : nullptr; On 2016/06/15 15:56:12, chrisha (slow) wrote: > Can g_browser_process every not exist in this context? ie: it's likely safe to > simply: > > DCHECK(g_browser_process); > memory::TabManager* tab_manager = g_browser_process->GetTabManager(); Looking at similar code, we can even forgo the DCHECK here and just assume the pointer valid.
PTAL https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:1002: // If tab_manager is not present, none tabs are discarded and therefore On 2016/06/15 15:56:12, chrisha (slow) wrote: > ... no* tabs... Acknowledged. https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:1004: // check the matching of the parameter and the status on tab_manager. On 2016/06/15 15:56:12, chrisha (slow) wrote: > Maybe slightly clearer as follows, less lines, and less duplication: > > bool discarded = false; > if (tab_manager) > discarded = tab_manager->IsTabDiscarded(web_contents); > if (!MatchesBool(params->query_info.discarded.get(), discarded) > continue; Acknowledged. https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2067033002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_tab_util.cc:395: g_browser_process ? g_browser_process->GetTabManager() : nullptr; On 2016/06/15 17:25:42, Georges Khalil wrote: > On 2016/06/15 15:56:12, chrisha (slow) wrote: > > Can g_browser_process every not exist in this context? ie: it's likely safe to > > simply: > > > > DCHECK(g_browser_process); > > memory::TabManager* tab_manager = g_browser_process->GetTabManager(); > > Looking at similar code, we can even forgo the DCHECK here and just assume the > pointer valid. Acknowledged.
The code lgtm. I've got no knowledge of the Extension API process, so please be sure to loop in the appropriate people.
Alright, looks good to me as well, so time for an owner's review. https://codereview.chromium.org/2067033002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2067033002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1000: if (tab_manager) nit: bool discarded = tab_manager && tab_manager->IsTabDiscarded(web_contents); (Same as next file below)
andersoncss@google.com changed reviewers: + rockot@chromium.org
Hi Ken, Can you please take a look at this CL? We're adding a new property on chrome.tabs extension API. Thanks https://codereview.chromium.org/2067033002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2067033002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1000: if (tab_manager) On 2016/06/16 20:22:13, Georges Khalil wrote: > nit: bool discarded = tab_manager && tab_manager->IsTabDiscarded(web_contents); > (Same as next file below) Done. It's less intuitive thought in this case, that's why I did the way I did in the first place. But I think with the comment explaining it should be fine.
rockot@chromium.org changed reviewers: + rchrisha@chromium.org, rdevlin.cronin@chromium.org - chrisha@chromium.org, rockot@chromium.org
rockot@chromium.org changed reviewers: + chrisha@chromium.org, rockot@chromium.org
Adding Devlin instead. Seems like a small change but it still needs an API review IMHO.
On 2016/06/16 21:56:15, Ken Rockot wrote: > Adding Devlin instead. Seems like a small change but it still needs an API > review IMHO. I agree with Ken. At the very least, this warrants a linked bug that describes the use case.
Thanks, sounds good. We'll link what we have soon. Even though we send the design doc <http://go/tabmanager-api-design> with the additions we intend to make to chrome-eng-review@ and brettw@ approved, we were not sure of how to proceed with this code review. We found Ken in the closest OWNERS file. Also, about the API implementation I found this page <http://go/new-chrome-api> with guidelines for new API proposals. In our case we intend to add some properties and a new function only, do you know if we need to go through the same process? If there's something else that we are missing or might have not noticed, please let us know. We're guiding the implementation mostly based in existing code and this Implementation Guidelines page <http://dev.chromium.org/developers/design-documents/extensions/proposed-chang...> . Thanks On Thu, Jun 16, 2016 at 6:11 PM, <rdevlin.cronin@chromium.org> wrote: > On 2016/06/16 21:56:15, Ken Rockot wrote: > > Adding Devlin instead. Seems like a small change but it still needs an > API > > review IMHO. > > I agree with Ken. At the very least, this warrants a linked bug that > describes > the use case. > > https://codereview.chromium.org/2067033002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG= ========== to ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG=621070 ==========
Oh and we created a tracking bug for it: http://crbug.com/621070 On 2016/06/16 23:11:21, chromium-reviews wrote: > Thanks, sounds good. We'll link what we have soon. > Even though we send the design doc <http://go/tabmanager-api-design> with > the additions we intend to make to chrome-eng-review@ and brettw@ approved, > we were not sure of how to proceed with this code review. We found Ken in > the closest OWNERS file. > > Also, about the API implementation I found this page > <http://go/new-chrome-api> with guidelines for new API proposals. In our > case we intend to add some properties and a new function only, do you know > if we need to go through the same process? > If there's something else that we are missing or might have not noticed, > please let us know. We're guiding the implementation mostly based in > existing code and this Implementation Guidelines page > <http://dev.chromium.org/developers/design-documents/extensions/proposed-chang...> > . > > Thanks > > On Thu, Jun 16, 2016 at 6:11 PM, <mailto:rdevlin.cronin@chromium.org> wrote: > > > On 2016/06/16 21:56:15, Ken Rockot wrote: > > > Adding Devlin instead. Seems like a small change but it still needs an > > API > > > review IMHO. > > > > I agree with Ken. At the very least, this warrants a linked bug that > > describes > > the use case. > > > > https://codereview.chromium.org/2067033002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
This warrants a test or two. https://codereview.chromium.org/2067033002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2067033002/diff/40001/chrome/common/extension... chrome/common/extensions/api/tabs.json:54: "discarded": {"type": "boolean", "description": "Whether the tab is discarded."}, specify what "discarded" means here. https://codereview.chromium.org/2067033002/diff/40001/chrome/common/extension... chrome/common/extensions/api/tabs.json:454: "description": "Whether the tabs are discarded." and here.
Included some tests. https://codereview.chromium.org/2067033002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2067033002/diff/40001/chrome/common/extension... chrome/common/extensions/api/tabs.json:54: "discarded": {"type": "boolean", "description": "Whether the tab is discarded."}, On 2016/06/17 17:49:26, Devlin wrote: > specify what "discarded" means here. Done. https://codereview.chromium.org/2067033002/diff/40001/chrome/common/extension... chrome/common/extensions/api/tabs.json:454: "description": "Whether the tabs are discarded." On 2016/06/17 17:49:26, Devlin wrote: > and here. Done.
Small addition to a test + nit. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:734: } I think we're missing one more transition: Add reloading of a discarded tab then make sure the numbers still make sense. https://codereview.chromium.org/2067033002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/common/extension... chrome/common/extensions/api/tabs.json:54: "discarded": {"type": "boolean", "description": "Whether the tab is discarded, meaning that its content has been unloaded from memory but the tab still exists in the tab-strip. The content will be reloaded if the tab gets activated."}, nit: I would rephrase this like this: Whether the tab is discarded. A discarded tab is one whose content has been unloaded from memory, but is still visible in the tab strip. Its content gets reloaded the next time it's activated. https://codereview.chromium.org/2067033002/diff/60001/chrome/common/extension... chrome/common/extensions/api/tabs.json:455: }, nit: same as above.
https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:220: content::WebContentsTester::CreateTestWebContents(profile(), nullptr); inline this, rather than assigning ownership and keeping the weak ptr. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:223: EXPECT_EQ(browser()->tab_strip_model()->GetActiveWebContents(), nit: switch the order here - expected, actual. Also, I'm not sure this check adds a ton of value (it should be tested in tab strip model and seems somewhat orthogonal here), but I don't feel strongly, so feel free to keep it. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:231: scoped_refptr<const Extension> extension_ = the '_' suffix is used when it's a private member variable. This is just stack-allocated, so simply "extension". (Also for tabs_list_ below.) https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:268: } It would be good to test the case of successfully retrieving discarded tabs.
On 2016/06/21 20:27:08, Devlin wrote: > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right): > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:220: > content::WebContentsTester::CreateTestWebContents(profile(), nullptr); > inline this, rather than assigning ownership and keeping the weak ptr. > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:223: > EXPECT_EQ(browser()->tab_strip_model()->GetActiveWebContents(), > nit: switch the order here - expected, actual. > > Also, I'm not sure this check adds a ton of value (it should be tested in tab > strip model and seems somewhat orthogonal here), but I don't feel strongly, so > feel free to keep it. > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:231: scoped_refptr<const > Extension> extension_ = > the '_' suffix is used when it's a private member variable. This is just > stack-allocated, so simply "extension". > > (Also for tabs_list_ below.) > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:268: } > It would be good to test the case of successfully retrieving discarded tabs. Whoops, hit publish too soon and didn't see all the files. Ignore for now.
Okay, ignore comment about testing retrieving discarded tabs. :) https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:640: ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); Do these have to be browser tests? Can we not put them in the unit test?
Thanks for reviewing. Have fixed for the previous comments. Lemme know if there's anything else. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:220: content::WebContentsTester::CreateTestWebContents(profile(), nullptr); On 2016/06/21 20:27:08, Devlin wrote: > inline this, rather than assigning ownership and keeping the weak ptr. Done. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:223: EXPECT_EQ(browser()->tab_strip_model()->GetActiveWebContents(), On 2016/06/21 20:27:08, Devlin wrote: > nit: switch the order here - expected, actual. > > Also, I'm not sure this check adds a ton of value (it should be tested in tab > strip model and seems somewhat orthogonal here), but I don't feel strongly, so > feel free to keep it. Done. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:231: scoped_refptr<const Extension> extension_ = On 2016/06/21 20:27:08, Devlin wrote: > the '_' suffix is used when it's a private member variable. This is just > stack-allocated, so simply "extension". > > (Also for tabs_list_ below.) Done. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:268: } On 2016/06/21 20:27:08, Devlin wrote: > It would be good to test the case of successfully retrieving discarded tabs. Acknowledged. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:640: ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); On 2016/06/21 20:31:51, Devlin wrote: > Do these have to be browser tests? Can we not put them in the unit test? We create these as browser tests because we need TabManager to be running in order to discard the tabs. TabManager lives in g_browser_process, which is running only in browser tests. https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:734: } On 2016/06/21 15:31:20, Georges Khalil wrote: > I think we're missing one more transition: Add reloading of a discarded tab then > make sure the numbers still make sense. Done. https://codereview.chromium.org/2067033002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/common/extension... chrome/common/extensions/api/tabs.json:54: "discarded": {"type": "boolean", "description": "Whether the tab is discarded, meaning that its content has been unloaded from memory but the tab still exists in the tab-strip. The content will be reloaded if the tab gets activated."}, On 2016/06/21 15:31:20, Georges Khalil wrote: > nit: I would rephrase this like this: > > Whether the tab is discarded. A discarded tab is one whose content has been > unloaded from memory, but is still visible in the tab strip. Its content gets > reloaded the next time it's activated. Done. https://codereview.chromium.org/2067033002/diff/60001/chrome/common/extension... chrome/common/extensions/api/tabs.json:455: }, On 2016/06/21 15:31:20, Georges Khalil wrote: > nit: same as above. Done.
https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:640: ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); On 2016/06/27 18:21:53, Anderson Silva wrote: > On 2016/06/21 20:31:51, Devlin wrote: > > Do these have to be browser tests? Can we not put them in the unit test? > > We create these as browser tests because we need TabManager to be running in > order to discard the tabs. TabManager lives in g_browser_process, which is > running only in browser tests. TestingBrowserProcess creates g_browser_process for tests, and we could modify it to conditionally have a TabManager. Are there any extra browser components that tab manager relies on that make this infeasible?
On 2016/06/28 20:14:30, Devlin wrote: > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/tabs/tabs_test.cc:640: > ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); > On 2016/06/27 18:21:53, Anderson Silva wrote: > > On 2016/06/21 20:31:51, Devlin wrote: > > > Do these have to be browser tests? Can we not put them in the unit test? > > > > We create these as browser tests because we need TabManager to be running in > > order to discard the tabs. TabManager lives in g_browser_process, which is > > running only in browser tests. > > TestingBrowserProcess creates g_browser_process for tests, and we could modify > it to conditionally have a TabManager. Are there any extra browser components > that tab manager relies on that make this infeasible? Hi Devlin, I think there's no other components besides TabManager. I had a look in TestingBrowserProcess but not sure how we would do this. I also tried different ways of testing this in an unittest and even though we can discard using a local tab_manager, this is not reflected in the Query API function because we also inquire TabManager from g_browser_process (line 971: https://codereview.chromium.org/2067033002/diff/80001/chrome/browser/extensio...). Do you think using TestingBrowserProcess we solve this? Thanks
On 2016/06/30 21:55:26, Anderson Silva wrote: > On 2016/06/28 20:14:30, Devlin wrote: > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > chrome/browser/extensions/api/tabs/tabs_test.cc:640: > > ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); > > On 2016/06/27 18:21:53, Anderson Silva wrote: > > > On 2016/06/21 20:31:51, Devlin wrote: > > > > Do these have to be browser tests? Can we not put them in the unit test? > > > > > > We create these as browser tests because we need TabManager to be running in > > > order to discard the tabs. TabManager lives in g_browser_process, which is > > > running only in browser tests. > > > > TestingBrowserProcess creates g_browser_process for tests, and we could modify > > it to conditionally have a TabManager. Are there any extra browser components > > that tab manager relies on that make this infeasible? > > Hi Devlin, > I think there's no other components besides TabManager. I had a look in > TestingBrowserProcess but not sure how we would do this. > I also tried different ways of testing this in an unittest and even though we > can discard using a local tab_manager, this is not reflected in the Query API > function because we also inquire TabManager from g_browser_process (line 971: > https://codereview.chromium.org/2067033002/diff/80001/chrome/browser/extensio...). > > Do you think using TestingBrowserProcess we solve this? > Thanks My thought was that we could add something like TestingBrowserProcess::SetTabManager(TabManager* tab_manager) and adjust TestingBrowserProcess::tab_manager() to return that if it's set. Without having much knowledge of the TabManager code, it seems like that would work. Are there other complications we'll run into doing that?
On 2016/06/30 22:23:20, Devlin wrote: > On 2016/06/30 21:55:26, Anderson Silva wrote: > > On 2016/06/28 20:14:30, Devlin wrote: > > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > > > > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > > chrome/browser/extensions/api/tabs/tabs_test.cc:640: > > > ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); > > > On 2016/06/27 18:21:53, Anderson Silva wrote: > > > > On 2016/06/21 20:31:51, Devlin wrote: > > > > > Do these have to be browser tests? Can we not put them in the unit > test? > > > > > > > > We create these as browser tests because we need TabManager to be running > in > > > > order to discard the tabs. TabManager lives in g_browser_process, which is > > > > running only in browser tests. > > > > > > TestingBrowserProcess creates g_browser_process for tests, and we could > modify > > > it to conditionally have a TabManager. Are there any extra browser > components > > > that tab manager relies on that make this infeasible? > > > > Hi Devlin, > > I think there's no other components besides TabManager. I had a look in > > TestingBrowserProcess but not sure how we would do this. > > I also tried different ways of testing this in an unittest and even though we > > can discard using a local tab_manager, this is not reflected in the Query API > > function because we also inquire TabManager from g_browser_process (line 971: > > > https://codereview.chromium.org/2067033002/diff/80001/chrome/browser/extensio...). > > > > Do you think using TestingBrowserProcess we solve this? > > Thanks > > My thought was that we could add something like > TestingBrowserProcess::SetTabManager(TabManager* tab_manager) and adjust > TestingBrowserProcess::tab_manager() to return that if it's set. Without having > much knowledge of the TabManager code, it seems like that would work. Are there > other complications we'll run into doing that? We changed TestingBrowserProcess to return TabManager. You can see the CL for that here: http://crrev.com/2121403002. I was then able to transform the browsertest to a unittest. Please take a look.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
On 2016/07/11 14:08:04, Anderson Silva wrote: > On 2016/06/30 22:23:20, Devlin wrote: > > On 2016/06/30 21:55:26, Anderson Silva wrote: > > > On 2016/06/28 20:14:30, Devlin wrote: > > > > > > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > > > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > > > chrome/browser/extensions/api/tabs/tabs_test.cc:640: > > > > ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); > > > > On 2016/06/27 18:21:53, Anderson Silva wrote: > > > > > On 2016/06/21 20:31:51, Devlin wrote: > > > > > > Do these have to be browser tests? Can we not put them in the unit > > test? > > > > > > > > > > We create these as browser tests because we need TabManager to be > running > > in > > > > > order to discard the tabs. TabManager lives in g_browser_process, which > is > > > > > running only in browser tests. > > > > > > > > TestingBrowserProcess creates g_browser_process for tests, and we could > > modify > > > > it to conditionally have a TabManager. Are there any extra browser > > components > > > > that tab manager relies on that make this infeasible? > > > > > > Hi Devlin, > > > I think there's no other components besides TabManager. I had a look in > > > TestingBrowserProcess but not sure how we would do this. > > > I also tried different ways of testing this in an unittest and even though > we > > > can discard using a local tab_manager, this is not reflected in the Query > API > > > function because we also inquire TabManager from g_browser_process (line > 971: > > > > > > https://codereview.chromium.org/2067033002/diff/80001/chrome/browser/extensio...). > > > > > > Do you think using TestingBrowserProcess we solve this? > > > Thanks > > > > My thought was that we could add something like > > TestingBrowserProcess::SetTabManager(TabManager* tab_manager) and adjust > > TestingBrowserProcess::tab_manager() to return that if it's set. Without > having > > much knowledge of the TabManager code, it seems like that would work. Are > there > > other complications we'll run into doing that? > > We changed TestingBrowserProcess to return TabManager. You can see the CL for > that here: http://crrev.com/2121403002. > I was then able to transform the browsertest to a unittest. > Please take a look. There's a mistake in the patch, please hold off.
Patchset #8 (id:180001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:200001) has been deleted
On 2016/07/11 14:45:02, Anderson Silva wrote: > On 2016/07/11 14:08:04, Anderson Silva wrote: > > On 2016/06/30 22:23:20, Devlin wrote: > > > On 2016/06/30 21:55:26, Anderson Silva wrote: > > > > On 2016/06/28 20:14:30, Devlin wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > > > > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2067033002/diff/60001/chrome/browser/extensio... > > > > > chrome/browser/extensions/api/tabs/tabs_test.cc:640: > > > > > ASSERT_TRUE(g_browser_process && g_browser_process->GetTabManager()); > > > > > On 2016/06/27 18:21:53, Anderson Silva wrote: > > > > > > On 2016/06/21 20:31:51, Devlin wrote: > > > > > > > Do these have to be browser tests? Can we not put them in the unit > > > test? > > > > > > > > > > > > We create these as browser tests because we need TabManager to be > > running > > > in > > > > > > order to discard the tabs. TabManager lives in g_browser_process, > which > > is > > > > > > running only in browser tests. > > > > > > > > > > TestingBrowserProcess creates g_browser_process for tests, and we could > > > modify > > > > > it to conditionally have a TabManager. Are there any extra browser > > > components > > > > > that tab manager relies on that make this infeasible? > > > > > > > > Hi Devlin, > > > > I think there's no other components besides TabManager. I had a look in > > > > TestingBrowserProcess but not sure how we would do this. > > > > I also tried different ways of testing this in an unittest and even though > > we > > > > can discard using a local tab_manager, this is not reflected in the Query > > API > > > > function because we also inquire TabManager from g_browser_process (line > > 971: > > > > > > > > > > https://codereview.chromium.org/2067033002/diff/80001/chrome/browser/extensio...). > > > > > > > > Do you think using TestingBrowserProcess we solve this? > > > > Thanks > > > > > > My thought was that we could add something like > > > TestingBrowserProcess::SetTabManager(TabManager* tab_manager) and adjust > > > TestingBrowserProcess::tab_manager() to return that if it's set. Without > > having > > > much knowledge of the TabManager code, it seems like that would work. Are > > there > > > other complications we'll run into doing that? > > > > We changed TestingBrowserProcess to return TabManager. You can see the CL for > > that here: http://crrev.com/2121403002. > > I was then able to transform the browsertest to a unittest. > > Please take a look. > > There's a mistake in the patch, please hold off. It's fine now. Thanks.
Unittest looks great, thank you for doing that! https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right): https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:219: TEST_F(TabsApiUnitTest, QueryDiscarded) { It doesn't look like this test has been formatted probably. Can you run git cl format? https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:224: content::OpenURLParams params_1(GURL(url::kAboutBlankURL), optional nit: I'd also probably just inline these, but I don't feel strongly. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:243: EXPECT_EQ(3u, result.get()->GetSize()); unique_ptr::operator-> forwards to the underlying ptr, so .get() is unnecessary. (Goes for all of these.) https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:271: EXPECT_EQ(2u, result.get()->GetSize()); It might be worth checking that the tabs you get are the correct ones. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:314: auto tsm = browser()->tab_strip_model(); style guide says to avoid using abbreviation like this. Prefer "tab_strip_model". Also, it's not immediately clear what type of object is returned from tab_strip_model() (ptr? refptr? ref?), so I think the auto here reduces readability. tl;dr: TabStripModel* tab_strip_model = browser()->tab_strip_model(). https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:341: browser()->tab_strip_model()->CloseAllTabs(); Is this necessary?
ptal https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right): https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:219: TEST_F(TabsApiUnitTest, QueryDiscarded) { On 2016/07/11 16:32:28, Devlin wrote: > It doesn't look like this test has been formatted probably. Can you run git cl > format? Done. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:224: content::OpenURLParams params_1(GURL(url::kAboutBlankURL), On 2016/07/11 16:32:28, Devlin wrote: > optional nit: I'd also probably just inline these, but I don't feel strongly. Acknowledged. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:243: EXPECT_EQ(3u, result.get()->GetSize()); On 2016/07/11 16:32:28, Devlin wrote: > unique_ptr::operator-> forwards to the underlying ptr, so .get() is unnecessary. > (Goes for all of these.) Done. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:271: EXPECT_EQ(2u, result.get()->GetSize()); On 2016/07/11 16:32:28, Devlin wrote: > It might be worth checking that the tabs you get are the correct ones. Done. Checked in 2 results. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:314: auto tsm = browser()->tab_strip_model(); On 2016/07/11 16:32:28, Devlin wrote: > style guide says to avoid using abbreviation like this. Prefer > "tab_strip_model". Also, it's not immediately clear what type of object is > returned from tab_strip_model() (ptr? refptr? ref?), so I think the auto here > reduces readability. > > tl;dr: TabStripModel* tab_strip_model = browser()->tab_strip_model(). Done. https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:341: browser()->tab_strip_model()->CloseAllTabs(); On 2016/07/11 16:32:28, Devlin wrote: > Is this necessary? Yes. Otherwise we crash at tear down.
modified comment for better explanation https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right): https://codereview.chromium.org/2067033002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:341: browser()->tab_strip_model()->CloseAllTabs(); On 2016/07/11 16:32:28, Devlin wrote: > Is this necessary? Yes. Because there's a check to make sure that TabStripModel is empty on tear down.
lgtm; thanks for all your work here! https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:997: // the tab is returned only if query_info.discarded is false. Otherwise nit: this comment is a bit long for what it's saying. Instead, let's say something like: // If tab_manager isn't present, then no tabs are discarded. The bit about matching the bool is both documented in the code itself and the norm for this function. https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_tab_util.cc:393: memory::TabManager* tab_manager = g_browser_process->GetTabManager(); it might be worth adding simple test for this (even as part of the existing one).
thanks for reviewing Davlin! I added the piece of code for testing tab_creation. https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:997: // the tab is returned only if query_info.discarded is false. Otherwise On 2016/07/11 20:25:04, Devlin wrote: > nit: this comment is a bit long for what it's saying. Instead, let's say > something like: > // If tab_manager isn't present, then no tabs are discarded. > > The bit about matching the bool is both documented in the code itself and the > norm for this function. Done. https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/2067033002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_tab_util.cc:393: memory::TabManager* tab_manager = g_browser_process->GetTabManager(); On 2016/07/11 20:25:04, Devlin wrote: > it might be worth adding simple test for this (even as part of the existing > one). Done.
Patchset #11 (id:300001) has been deleted
It doesn't seem as if we'll be able to convert that test into a unit test after all, as the test harness uses an IO messageloop whereas TabManager requires running on a UI thread. Curiously, the other unit tests give a non-fatal error (Cannot create windows on non-UI thread!) as they're spawning a window in a IO thread. I'm surprised they don't crash. I suggest we revert back to using a browser test for our test. WDYT?
On 2016/07/13 13:33:52, Georges Khalil wrote: > It doesn't seem as if we'll be able to convert that test into a unit test after > all, as the test harness uses an IO messageloop whereas TabManager requires > running on a UI thread. > > Curiously, the other unit tests give a non-fatal error (Cannot create windows on > non-UI thread!) as they're spawning a window in a IO thread. I'm surprised they > don't crash. > > I suggest we revert back to using a browser test for our test. > > WDYT? What if you add a call to ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);?
On 2016/07/13 15:04:55, Devlin wrote: > On 2016/07/13 13:33:52, Georges Khalil wrote: > > It doesn't seem as if we'll be able to convert that test into a unit test > after > > all, as the test harness uses an IO messageloop whereas TabManager requires > > running on a UI thread. > > > > Curiously, the other unit tests give a non-fatal error (Cannot create windows > on > > non-UI thread!) as they're spawning a window in a IO thread. I'm surprised > they > > don't crash. > > > > I suggest we revert back to using a browser test for our test. > > > > WDYT? > > What if you add a call to > ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);? Yeah we tried that, but then JsonPrefStore crashes (important_file_writer.cc(176)] Check failed: false). We've spent some time on it now, to no avail :(
On 2016/07/13 15:26:05, Georges Khalil wrote: > On 2016/07/13 15:04:55, Devlin wrote: > > On 2016/07/13 13:33:52, Georges Khalil wrote: > > > It doesn't seem as if we'll be able to convert that test into a unit test > > after > > > all, as the test harness uses an IO messageloop whereas TabManager requires > > > running on a UI thread. > > > > > > Curiously, the other unit tests give a non-fatal error (Cannot create > windows > > on > > > non-UI thread!) as they're spawning a window in a IO thread. I'm surprised > > they > > > don't crash. > > > > > > I suggest we revert back to using a browser test for our test. > > > > > > WDYT? > > > > What if you add a call to > > ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);? > > Yeah we tried that, but then JsonPrefStore crashes > (important_file_writer.cc(176)] Check failed: false). > > We've spent some time on it now, to no avail :( Dang. There's probably a way of getting it all to work, but I don't want you to shave any more yaks than you already have. Reverting to a browsertest is fine.
On 2016/07/13 16:03:46, Devlin wrote: > On 2016/07/13 15:26:05, Georges Khalil wrote: > > On 2016/07/13 15:04:55, Devlin wrote: > > > On 2016/07/13 13:33:52, Georges Khalil wrote: > > > > It doesn't seem as if we'll be able to convert that test into a unit test > > > after > > > > all, as the test harness uses an IO messageloop whereas TabManager > requires > > > > running on a UI thread. > > > > > > > > Curiously, the other unit tests give a non-fatal error (Cannot create > > windows > > > on > > > > non-UI thread!) as they're spawning a window in a IO thread. I'm surprised > > > they > > > > don't crash. > > > > > > > > I suggest we revert back to using a browser test for our test. > > > > > > > > WDYT? > > > > > > What if you add a call to > > > ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);? > > > > Yeah we tried that, but then JsonPrefStore crashes > > (important_file_writer.cc(176)] Check failed: false). > > > > We've spent some time on it now, to no avail :( > > Dang. There's probably a way of getting it all to work, but I don't want you to > shave any more yaks than you already have. Reverting to a browsertest is fine. Yeah, we really wanted to get the unit tests working, but I think at this point, we'll revert to browser tests. We'll put a TODO in the code to reexamine this down the road.
On 2016/07/13 17:32:41, Georges Khalil wrote: > On 2016/07/13 16:03:46, Devlin wrote: > > On 2016/07/13 15:26:05, Georges Khalil wrote: > > > On 2016/07/13 15:04:55, Devlin wrote: > > > > On 2016/07/13 13:33:52, Georges Khalil wrote: > > > > > It doesn't seem as if we'll be able to convert that test into a unit > test > > > > after > > > > > all, as the test harness uses an IO messageloop whereas TabManager > > requires > > > > > running on a UI thread. > > > > > > > > > > Curiously, the other unit tests give a non-fatal error (Cannot create > > > windows > > > > on > > > > > non-UI thread!) as they're spawning a window in a IO thread. I'm > surprised > > > > they > > > > > don't crash. > > > > > > > > > > I suggest we revert back to using a browser test for our test. > > > > > > > > > > WDYT? > > > > > > > > What if you add a call to > > > > ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);? > > > > > > Yeah we tried that, but then JsonPrefStore crashes > > > (important_file_writer.cc(176)] Check failed: false). > > > > > > We've spent some time on it now, to no avail :( > > > > Dang. There's probably a way of getting it all to work, but I don't want you > to > > shave any more yaks than you already have. Reverting to a browsertest is > fine. > > Yeah, we really wanted to get the unit tests working, but I think at this point, > we'll revert to browser tests. We'll put a TODO in the code to reexamine this > down the road. Sounds good. Thank you for the in-depth investigation, and sorry it didn't work out.
On 2016/07/13 18:27:30, Devlin wrote: > On 2016/07/13 17:32:41, Georges Khalil wrote: > > On 2016/07/13 16:03:46, Devlin wrote: > > > On 2016/07/13 15:26:05, Georges Khalil wrote: > > > > On 2016/07/13 15:04:55, Devlin wrote: > > > > > On 2016/07/13 13:33:52, Georges Khalil wrote: > > > > > > It doesn't seem as if we'll be able to convert that test into a unit > > test > > > > > after > > > > > > all, as the test harness uses an IO messageloop whereas TabManager > > > requires > > > > > > running on a UI thread. > > > > > > > > > > > > Curiously, the other unit tests give a non-fatal error (Cannot create > > > > windows > > > > > on > > > > > > non-UI thread!) as they're spawning a window in a IO thread. I'm > > surprised > > > > > they > > > > > > don't crash. > > > > > > > > > > > > I suggest we revert back to using a browser test for our test. > > > > > > > > > > > > WDYT? > > > > > > > > > > What if you add a call to > > > > > ResetThreadBundle(content::TestBrowserThreadBundle::DEFAULT);? > > > > > > > > Yeah we tried that, but then JsonPrefStore crashes > > > > (important_file_writer.cc(176)] Check failed: false). > > > > > > > > We've spent some time on it now, to no avail :( > > > > > > Dang. There's probably a way of getting it all to work, but I don't want > you > > to > > > shave any more yaks than you already have. Reverting to a browsertest is > > fine. > > > > Yeah, we really wanted to get the unit tests working, but I think at this > point, > > we'll revert to browser tests. We'll put a TODO in the code to reexamine this > > down the road. > > Sounds good. Thank you for the in-depth investigation, and sorry it didn't work > out. Changed it back to browsertest and added a TODO so we can lookup this again later. It passed in all trybots but let me know there are any concerns, otherwise I'll submit it. Thanks
lgtm with optional nit https://codereview.chromium.org/2067033002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2067033002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1335: utils::ToList(utils::RunFunctionAndReturnSingleResult( optional nit: These lines pretty much always look the same. You could add a lambda up at the top to clear this up: // Note I moved this up here because there's no need to recreate an // extension each call. scoped_refptr<const Extension> extension = test_util::CreateEmptyExtension(); auto RunQueryFunction = [&this, &extension](const char* query_info) { scoped_refptr<TabsQueryFunction> function = new TabsQueryFunction(); function->set_extension(extension.get()); return utils::ToList(utils::RunFunctionAndReturnSingleResult( function.get(), query_info, browser()))); }; Then these blocks become: { std::unique_ptr<base::ListValue> result = RunQueryFunction("[{\"discarded\": false}]"); EXPECT_EQ(3u, result->GetSize()); }
Added the lambda expression. Committing to CQ now. Thanks a lot for reviewing. https://codereview.chromium.org/2067033002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2067033002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1335: utils::ToList(utils::RunFunctionAndReturnSingleResult( On 2016/07/13 20:17:46, Devlin wrote: > optional nit: These lines pretty much always look the same. You could add a > lambda up at the top to clear this up: > // Note I moved this up here because there's no need to recreate an > // extension each call. > scoped_refptr<const Extension> extension = > test_util::CreateEmptyExtension(); > auto RunQueryFunction = [&this, &extension](const char* query_info) { > scoped_refptr<TabsQueryFunction> function = new TabsQueryFunction(); > function->set_extension(extension.get()); > return utils::ToList(utils::RunFunctionAndReturnSingleResult( > function.get(), query_info, browser()))); > }; > > Then these blocks become: > { > std::unique_ptr<base::ListValue> result = > RunQueryFunction("[{\"discarded\": false}]"); > EXPECT_EQ(3u, result->GetSize()); > } Done. That's very nice. I didn't know about lambda expressions.
The CQ bit was checked by andersoncss@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2067033002/#ps340001 (title: "added lambda expression to simplify test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG=621070 ========== to ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG=621070 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG=621070 ========== to ========== Adding new Discarded property on Tab type and query() function on existing Tabs API. BUG=621070 Committed: https://crrev.com/12caff1b13a6b2b7721897240aaa143fc8714276 Cr-Commit-Position: refs/heads/master@{#405492} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/12caff1b13a6b2b7721897240aaa143fc8714276 Cr-Commit-Position: refs/heads/master@{#405492} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
