Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2142413003:
Adding Discarded property support for onUpdate function. (Closed)
|
Created:
4 years, 5 months ago by Anderson Silva Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@API_Discarded Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding Discarded property support for onUpdate function.
BUG=621070
Committed: https://crrev.com/94afe44e219ad774df9290dd980d6214141a089a
Cr-Commit-Position: refs/heads/master@{#408822}
Patch Set 1 #
Total comments: 6
Patch Set 2 : fixed nit #Patch Set 3 : merged TOT #Patch Set 4 : test for onUpdated event of discardable property #Patch Set 5 : nit missing end of line #
Total comments: 17
Patch Set 6 : fixed comments #Patch Set 7 : todo #
Total comments: 10
Patch Set 8 : nits fixed #
Total comments: 4
Patch Set 9 : nit fixed #Messages
Total messages: 32 (12 generated)
Description was changed from ========== Adding Discarded property support for onUpdate function. BUG=621070 ========== to ========== Adding Discarded property support for onUpdate function. BUG=621070 ==========
andersoncss@google.com changed reviewers: + chrisha@chromium.org, georgesak@chromium.org
ptal
Nice! Only nits so far. Now we need test coverage. https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_event_router.cc:564: TabStripModel* tab_strip = NULL; nit: nullptr https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_event_router.cc:565: int tab_index; nit: initialize. https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_event_router.h:94: void OnDiscardedStateChange(content::WebContents* contents, nit: memory::TabManagerObserver:
fixed nit. looking at tests https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_event_router.cc:564: TabStripModel* tab_strip = NULL; On 2016/07/13 15:52:10, Georges Khalil wrote: > nit: nullptr Done. https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_event_router.cc:565: int tab_index; On 2016/07/13 15:52:10, Georges Khalil wrote: > nit: initialize. Done. https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/2142413003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_event_router.h:94: void OnDiscardedStateChange(content::WebContents* contents, On 2016/07/13 15:52:10, Georges Khalil wrote: > nit: memory::TabManagerObserver: Done.
Hi Devlin, can you please take a look at this as well? Even though we test that TabManagerObserver observers are notified of events, for this piece of code we need to test that extensions are listening when discarded state is changed and there is a subscriber for chrome.tabs.onUpdated(). We've dug into it and found out we can have extension API tests as described here: https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/REA.... The problem is that we need the other part of the API (chrome.tabs.discard()) as well in order to implement such tests. So, after discussing with Georges offline, our plan is to finish with Discarded property here and then implement tabs.discard() with extension tests for it and for onUpdate() of Discarded as well. Thanks
andersoncss@google.com changed reviewers: + rdevlin.cronin@chromium.org
On 2016/07/14 17:48:22, Anderson Silva wrote: > Hi Devlin, can you please take a look at this as well? > > Even though we test that TabManagerObserver observers are notified of events, > for this piece of code we need to test that extensions are listening when > discarded state is changed and there is a subscriber for > chrome.tabs.onUpdated(). > > We've dug into it and found out we can have extension API tests as described > here: > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/REA.... > The problem is that we need the other part of the API (chrome.tabs.discard()) as > well in order to implement such tests. > > So, after discussing with Georges offline, our plan is to finish with Discarded > property here and then implement tabs.discard() with extension tests for it and > for onUpdate() of Discarded as well. > > Thanks mailing message
On 2016/07/14 17:52:32, Anderson Silva wrote: > On 2016/07/14 17:48:22, Anderson Silva wrote: > > Hi Devlin, can you please take a look at this as well? > > > > Even though we test that TabManagerObserver observers are notified of events, > > for this piece of code we need to test that extensions are listening when > > discarded state is changed and there is a subscriber for > > chrome.tabs.onUpdated(). > > > > We've dug into it and found out we can have extension API tests as described > > here: > > > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/REA.... > > The problem is that we need the other part of the API (chrome.tabs.discard()) > as > > well in order to implement such tests. > > > > So, after discussing with Georges offline, our plan is to finish with > Discarded > > property here and then implement tabs.discard() with extension tests for it > and > > for onUpdate() of Discarded as well. > > > > Thanks > > mailing message This needs to be updated because we discovered that the unittest wasn't viable, right? Also, somewhat unrelated, but is there a reason to have chrome.tabs.discard() instead of chrome.tabs.update(tabId, {discarded: true})?
On 2016/07/15 15:12:08, Devlin wrote: > On 2016/07/14 17:52:32, Anderson Silva wrote: > > On 2016/07/14 17:48:22, Anderson Silva wrote: > > > Hi Devlin, can you please take a look at this as well? > > > > > > Even though we test that TabManagerObserver observers are notified of > events, > > > for this piece of code we need to test that extensions are listening when > > > discarded state is changed and there is a subscriber for > > > chrome.tabs.onUpdated(). > > > > > > We've dug into it and found out we can have extension API tests as described > > > here: > > > > > > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/REA.... > > > The problem is that we need the other part of the API > (chrome.tabs.discard()) > > as > > > well in order to implement such tests. > > > > > > So, after discussing with Georges offline, our plan is to finish with > > Discarded > > > property here and then implement tabs.discard() with extension tests for it > > and > > > for onUpdate() of Discarded as well. > > > > > > Thanks > > > > mailing message > > This needs to be updated because we discovered that the unittest wasn't viable, > right? > > Also, somewhat unrelated, but is there a reason to have chrome.tabs.discard() > instead of chrome.tabs.update(tabId, {discarded: true})? To test this new property, we would need to be able to discard from the extension side as well, which is why we propose to land this and then, the following CL will implement discard() and have a comprehensive test for both the funcitonality and the property. Also, the reason we want to have a call for discarding is that the action is not guaranteed to be successful. Discarded is the property that reflects the status that can be checked and discard() is the API call to invoke the mechanism. Also, this allows the extension to ask Chrome to discard the least important tab, as per the internal ranking (so the extension need not specify a specific tab, Chrome will choose it). I hope that clears it up! Let me know if you have more question :)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by andersoncss@google.com 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...
Hi Devlin, Can you please take a look at this CL? Now that we have chrome.tabs.discard() in, we can test onUpdated for the discarded property. Thanks
Mostly just small stuff. https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:146: if (g_browser_process->GetTabManager()) Document when this is null. https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:147: g_browser_process->GetTabManager()->AddObserver(this); Use a ScopedObserver. https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:152: g_browser_process->GetTabManager()->RemoveObserver(this); Is the TabManager guaranteed to outlive the TabsEventRouter (in the case that it exists at all)? Similarly, if it *will* ever exist, is it guaranteed to exist at this object's construction time? https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:567: if (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) { Why do we use this method here instead of e.g. GetTabId? https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.html (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.html:2: * Copyright (c) 2016 The Chromium Authors. All rights reserved. Use of this nit: no (c) https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. ditto https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:44: // Discard and update testTab (the id changes after a tab is discarded). The id changes? That's a bit yucky. We normally guarantee the id to remain constant for a tab's lifetime. Is there a reason it needs to change?
https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:146: if (g_browser_process->GetTabManager()) On 2016/07/29 16:01:09, Devlin wrote: > Document when this is null. It shouldn't be null anymore as we included a tab_manager for TestingBrowserProcess. https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:147: g_browser_process->GetTabManager()->AddObserver(this); On 2016/07/29 16:01:09, Devlin wrote: > Use a ScopedObserver. Done. https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:152: g_browser_process->GetTabManager()->RemoveObserver(this); On 2016/07/29 16:01:09, Devlin wrote: > Is the TabManager guaranteed to outlive the TabsEventRouter (in the case that it > exists at all)? Similarly, if it *will* ever exist, is it guaranteed to exist > at this object's construction time? TabManager is created at the first time GetTabManager() is invoked, so it'll always exist. As it lives in g_browser_process, it should also outlive TabsEventRouter. However, as I added ScopedObserver now, this shouldn't be an issue anymore. https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:567: if (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) { On 2016/07/29 16:01:09, Devlin wrote: > Why do we use this method here instead of e.g. GetTabId? I see this call just as a check that we can find a the tab strip for this web_contents. We won't really use any of the information, we'll just make sure there's a tab strip associated with this contents before sending events. https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.html (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.html:2: * Copyright (c) 2016 The Chromium Authors. All rights reserved. Use of this On 2016/07/29 16:01:09, Devlin wrote: > nit: no (c) Done. https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/29 16:01:09, Devlin wrote: > ditto Done. https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:44: // Discard and update testTab (the id changes after a tab is discarded). On 2016/07/29 16:01:09, Devlin wrote: > The id changes? That's a bit yucky. We normally guarantee the id to remain > constant for a tab's lifetime. Is there a reason it needs to change? Yeah, you're right, this is weird. But it isn't related to this, it's an issue because when the tab is discarded the old web_contents is deleted and a new one is created with a new assigned id. We'll look deeper on this issue to try to find if we can maintain the id.
(Just responding) https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:152: g_browser_process->GetTabManager()->RemoveObserver(this); On 2016/07/29 19:05:59, Anderson Silva wrote: > On 2016/07/29 16:01:09, Devlin wrote: > > Is the TabManager guaranteed to outlive the TabsEventRouter (in the case that > it > > exists at all)? Similarly, if it *will* ever exist, is it guaranteed to exist > > at this object's construction time? > > TabManager is created at the first time GetTabManager() is invoked, so it'll > always exist. As it lives in g_browser_process, it should also outlive > TabsEventRouter. > However, as I added ScopedObserver now, this shouldn't be an issue anymore. Not strictly true. A ScopedObserver just automatically calls RemoveObserver() on any sources that are still registered when it's destructed; it doesn't prevent a UAF if the owning object outlives the object it's observing. (But given the TabManager is only destructed when the browser process is, we should be okay.) https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_event_router.cc:567: if (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) { On 2016/07/29 19:05:59, Anderson Silva wrote: > On 2016/07/29 16:01:09, Devlin wrote: > > Why do we use this method here instead of e.g. GetTabId? > > I see this call just as a check that we can find a the tab strip for this > web_contents. We won't really use any of the information, we'll just make sure > there's a tab strip associated with this contents before sending events. Do we strictly need a tab strip associated? Or can we just check that there's an ID associated? https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:44: // Discard and update testTab (the id changes after a tab is discarded). On 2016/07/29 19:05:59, Anderson Silva wrote: > On 2016/07/29 16:01:09, Devlin wrote: > > The id changes? That's a bit yucky. We normally guarantee the id to remain > > constant for a tab's lifetime. Is there a reason it needs to change? > > Yeah, you're right, this is weird. But it isn't related to this, it's an issue > because when the tab is discarded the old web_contents is deleted and a new one > is created with a new assigned id. > We'll look deeper on this issue to try to find if we can maintain the id. Cool, thanks! I don't want to block this issue on that, but can you add a TODO here (and maybe file a bug if there isn't one?) so that we can track this?
On 2016/07/29 20:17:33, Devlin wrote: > (Just responding) > > https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): > > https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_event_router.cc:152: > g_browser_process->GetTabManager()->RemoveObserver(this); > On 2016/07/29 19:05:59, Anderson Silva wrote: > > On 2016/07/29 16:01:09, Devlin wrote: > > > Is the TabManager guaranteed to outlive the TabsEventRouter (in the case > that > > it > > > exists at all)? Similarly, if it *will* ever exist, is it guaranteed to > exist > > > at this object's construction time? > > > > TabManager is created at the first time GetTabManager() is invoked, so it'll > > always exist. As it lives in g_browser_process, it should also outlive > > TabsEventRouter. > > However, as I added ScopedObserver now, this shouldn't be an issue anymore. > > Not strictly true. A ScopedObserver just automatically calls RemoveObserver() > on any sources that are still registered when it's destructed; it doesn't > prevent a UAF if the owning object outlives the object it's observing. (But > given the TabManager is only destructed when the browser process is, we should > be okay.) I see. Good, thanks. > https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_event_router.cc:567: if > (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) { > On 2016/07/29 19:05:59, Anderson Silva wrote: > > On 2016/07/29 16:01:09, Devlin wrote: > > > Why do we use this method here instead of e.g. GetTabId? > > > > I see this call just as a check that we can find a the tab strip for this > > web_contents. We won't really use any of the information, we'll just make sure > > there's a tab strip associated with this contents before sending events. > > Do we strictly need a tab strip associated? Or can we just check that there's > an ID associated? I think this function could just be: std::set<std::string> changed_property_names; changed_property_names.insert(tabs_constants::kDiscardedKey); DispatchTabUpdatedEvent(contents, std::move(changed_property_names)); But I check if there's a tab strip because I don't think we need to dispatch events if there is no TabStrip. But I'm not strong about this, since I followed what I found in other events dispatching functions (https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...). And I don't think checking just checking that there's an id would have the same effect since a web_contents can have ids but not be associated with a TabStrip. But let me know, because I don't have deep knowledge of how this work altogether. > > https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... > File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): > > https://codereview.chromium.org/2142413003/diff/100001/chrome/test/data/exten... > chrome/test/data/extensions/api_test/tabs/basics/discarded.js:44: // Discard and > update testTab (the id changes after a tab is discarded). > On 2016/07/29 19:05:59, Anderson Silva wrote: > > On 2016/07/29 16:01:09, Devlin wrote: > > > The id changes? That's a bit yucky. We normally guarantee the id to remain > > > constant for a tab's lifetime. Is there a reason it needs to change? > > > > Yeah, you're right, this is weird. But it isn't related to this, it's an issue > > because when the tab is discarded the old web_contents is deleted and a new > one > > is created with a new assigned id. > > We'll look deeper on this issue to try to find if we can maintain the id. > > Cool, thanks! I don't want to block this issue on that, but can you add a TODO > here (and maybe file a bug if there isn't one?) so that we can track this? Just did. I've put in you in cc so you can follow along as well. Thanks.
Cool! Just some last nits, mostly around confusing JS formatting. (We're already pretty inconsistent in tests, but I'm trying to get everyone to use a similar style) https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:12: pass(function (winId, tabIds) { indent parameters more, function body less: createWindow([...], {}, pass(function(winId, tabIds) { testTabId = tabIds[1]; })); https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:16: waitForAllTabs(pass(function () { no space between function and ( https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:29: function (tabId, changeInfo, tab) { indent these two more, outdent the function body two. So it should be: var onUpdatedCompleted = chrome.test.listenForever( chrome.tabs.onUpdated, function(tabId, changeInfo, tab) { if (...) ... }); (Note also the removal of the ' ' between 'function' and '(' https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:42: ); nit: put the paren and the closing function brace on the same line, so this would look like: if (...) { ... } }); https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:58: chrome.tabs.onUpdated, see indentation advice for line 27
On 2016/07/29 20:48:11, Anderson Silva wrote: > https://codereview.chromium.org/2142413003/diff/100001/chrome/browser/extensi... > > chrome/browser/extensions/api/tabs/tabs_event_router.cc:567: if > > (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) { > > On 2016/07/29 19:05:59, Anderson Silva wrote: > > > On 2016/07/29 16:01:09, Devlin wrote: > > > > Why do we use this method here instead of e.g. GetTabId? > > > > > > I see this call just as a check that we can find a the tab strip for this > > > web_contents. We won't really use any of the information, we'll just make > sure > > > there's a tab strip associated with this contents before sending events. > > > > Do we strictly need a tab strip associated? Or can we just check that there's > > an ID associated? > > I think this function could just be: > > std::set<std::string> changed_property_names; > changed_property_names.insert(tabs_constants::kDiscardedKey); > DispatchTabUpdatedEvent(contents, std::move(changed_property_names)); > > But I check if there's a tab strip because I don't think we need to dispatch > events if there is no TabStrip. > But I'm not strong about this, since I followed what I found in other events > dispatching functions > (https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...). > And I don't think checking just checking that there's an id would have the same > effect since a web_contents can have ids but not be associated with a TabStrip. > > But let me know, because I don't have deep knowledge of how this work > altogether. I still think this is a bit unnecessary, but you're right that other events do it too. I'll dig into it later. This is fine for now.
https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:12: pass(function (winId, tabIds) { On 2016/07/29 21:23:40, Devlin wrote: > indent parameters more, function body less: > createWindow([...], {}, > pass(function(winId, tabIds) { > testTabId = tabIds[1]; > })); Done. https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:16: waitForAllTabs(pass(function () { On 2016/07/29 21:23:40, Devlin wrote: > no space between function and ( Done. https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:29: function (tabId, changeInfo, tab) { On 2016/07/29 21:23:40, Devlin wrote: > indent these two more, outdent the function body two. So it should be: > var onUpdatedCompleted = chrome.test.listenForever( > chrome.tabs.onUpdated, > function(tabId, changeInfo, tab) { > if (...) > ... > }); > > (Note also the removal of the ' ' between 'function' and '(' Done. https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:42: ); On 2016/07/29 21:23:40, Devlin wrote: > nit: put the paren and the closing function brace on the same line, so this > would look like: > if (...) { > ... > } > }); Done. https://codereview.chromium.org/2142413003/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:58: chrome.tabs.onUpdated, On 2016/07/29 21:23:40, Devlin wrote: > see indentation advice for line 27 Done.
The CQ bit was checked by andersoncss@google.com 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...
cool, lgtm! https://codereview.chromium.org/2142413003/diff/160001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:31: // Make sure it's the right tab. indentation is now off in this if block https://codereview.chromium.org/2142413003/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:60: // Make sure it's the right tab. ditto
The CQ bit was checked by andersoncss@google.com
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/2142413003/#ps180001 (title: "nit fixed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
great, thanks! sending to CQ https://codereview.chromium.org/2142413003/diff/160001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (right): https://codereview.chromium.org/2142413003/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:31: // Make sure it's the right tab. On 2016/07/29 22:23:25, Devlin wrote: > indentation is now off in this if block Done. https://codereview.chromium.org/2142413003/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:60: // Make sure it's the right tab. On 2016/07/29 22:23:25, Devlin wrote: > ditto Done.
Message was sent while issue was closed.
Description was changed from ========== Adding Discarded property support for onUpdate function. BUG=621070 ========== to ========== Adding Discarded property support for onUpdate function. BUG=621070 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adding Discarded property support for onUpdate function. BUG=621070 ========== to ========== Adding Discarded property support for onUpdate function. BUG=621070 Committed: https://crrev.com/94afe44e219ad774df9290dd980d6214141a089a Cr-Commit-Position: refs/heads/master@{#408822} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/94afe44e219ad774df9290dd980d6214141a089a Cr-Commit-Position: refs/heads/master@{#408822} |
