Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153943002:
Implementing TabManager extensions API Discard Function. (Closed)
|
Created:
4 years, 5 months ago by Anderson Silva Modified:
4 years, 4 months ago CC:
asvitkine+watch_chromium.org, 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. |
DescriptionImplementing TabManager extensions API Discard Function.
BUG=621070
Committed: https://crrev.com/71250fbd07f01e5f5f27bccce5ac23d4c94e3563
Cr-Commit-Position: refs/heads/master@{#408137}
Patch Set 1 #Patch Set 2 : fixing bug on DiscardTabImpl #Patch Set 3 : testcases #
Total comments: 17
Patch Set 4 : first round of comments fixed #
Total comments: 6
Patch Set 5 : secondary comments fixed #
Total comments: 10
Patch Set 6 : fixing comments #
Total comments: 23
Patch Set 7 : fixes #Patch Set 8 : nit on function description #
Total comments: 4
Patch Set 9 : changed returned tab to optional plus tests #
Total comments: 7
Patch Set 10 : fixes #Patch Set 11 : new error messages #
Total comments: 10
Patch Set 12 : nits fixed #Patch Set 13 : merged TOT #Patch Set 14 : reset protection time #Messages
Total messages: 67 (36 generated)
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...
andersoncss@google.com changed reviewers: + chrisha@chromium.org, georgesak@chromium.org
ptal tested in an interactive extension and looking into creating tests now but already starting reviewing cycle.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andersoncss@google.com to run a CQ dry run
tests included PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Good start! First round of comments. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2178: // Otherwise invokes discard function in TabManager with null web_contents nit: invokes -> invoke https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2183: &contents, nullptr, &error_)) nit: braces. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2194: success = false; This can be a single line: bool success = !!contents; https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2199: std::unique_ptr<api::tabs::Tab> tab(new api::tabs::Tab); A small comment would be good here, to explain how the callback works. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:326: ~TabsDiscardFunction() override {} missing private. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:111: // If |contents| is null, discards the least important tab using DiscardTab(). Start comment by explaining that this is the function that will be called by the extensions API. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:135: // Used in tets to change the protection time of the tabs. nit: tets -> tests. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:136: void set_minimum_protection_time_for_tests(unsigned int time_seconds); This should take a base::TimeDelta so there's no confusion. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:266: // Implementation of DiscardTab. Add comment about return value.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
first round of comments fixed! https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2178: // Otherwise invokes discard function in TabManager with null web_contents On 2016/07/19 19:39:45, Georges Khalil wrote: > nit: invokes -> invoke Done. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2183: &contents, nullptr, &error_)) On 2016/07/19 19:39:45, Georges Khalil wrote: > nit: braces. Done. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2194: success = false; On 2016/07/19 19:39:45, Georges Khalil wrote: > This can be a single line: > > bool success = !!contents; Done. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2199: std::unique_ptr<api::tabs::Tab> tab(new api::tabs::Tab); On 2016/07/19 19:39:45, Georges Khalil wrote: > A small comment would be good here, to explain how the callback works. Done. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:326: ~TabsDiscardFunction() override {} On 2016/07/19 19:39:45, Georges Khalil wrote: > missing private. Hmm. I think private is not necessary. Most of the functions in the API doesn't have it. Why do you think so? The functions above (Zoom API) might be private because it inherits from another class than ChromeSyncExtensionFunction. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:111: // If |contents| is null, discards the least important tab using DiscardTab(). On 2016/07/19 19:39:45, Georges Khalil wrote: > Start comment by explaining that this is the function that will be called by the > extensions API. Done. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:136: void set_minimum_protection_time_for_tests(unsigned int time_seconds); On 2016/07/19 19:39:45, Georges Khalil wrote: > This should take a base::TimeDelta so there's no confusion. Done.
lgtm % nits. https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:326: ~TabsDiscardFunction() override {} On 2016/07/19 20:28:42, Anderson Silva wrote: > On 2016/07/19 19:39:45, Georges Khalil wrote: > > missing private. > > Hmm. I think private is not necessary. Most of the functions in the API doesn't > have it. Why do you think so? > The functions above (Zoom API) might be private because it inherits from another > class than ChromeSyncExtensionFunction. Yeah, it seems to be inconsistent through out the file. I'll defer to the owner to make the call. https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2187: // Discards the tab. nit: discards -> discard. https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2192: bool success = !!contents; nit: move this lower (to where it's needed). https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:139: int* active_index, nit: is that the result of git cl format?
andersoncss@google.com changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin, Can you please take a look at this CL? Thanks https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2187: // Discards the tab. On 2016/07/19 20:35:40, Georges Khalil wrote: > nit: discards -> discard. Done. https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2192: bool success = !!contents; On 2016/07/19 20:35:40, Georges Khalil wrote: > nit: move this lower (to where it's needed). Done. https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:139: int* active_index, On 2016/07/19 20:35:40, Georges Khalil wrote: > nit: is that the result of git cl format? cl format doesn't fix that, but lint catchs it as and error since pointers should have no space after type name.
https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2189: if (!tab_manager) When can this be null? https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2193: if (!has_callback()) I understand the motivation for this, but if/when we move to promise-based apis, this will make the transition harder. Let's just create the result in all cases. Extensions shouldn't be calling discard very frequently. https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:325: class TabsDiscardFunction : public ChromeSyncExtensionFunction { ChromeSyncExtensionFunction is dead. Prefer UIThreadExtensionFunction. https://codereview.chromium.org/2153943002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2153943002/diff/80001/chrome/common/extension... chrome/common/extensions/api/tabs.json:914: "description": "Discards a tab from memory. Discarded tabs are still visible on the tab strip and get reloaded when clicked on.", s/clicked on/activated. https://codereview.chromium.org/2153943002/diff/80001/chrome/common/extension... chrome/common/extensions/api/tabs.json:937: "description": "Status describing if the tab was successfully discarded or not." Is there a reason for this boolean rather than using lastError?
https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2189: if (!tab_manager) On 2016/07/19 22:49:12, Devlin wrote: > When can this be null? It's null on non-supported platforms (Android, iOS). But as extensions are not supported on those, we can remove this. https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:2193: if (!has_callback()) On 2016/07/19 22:49:11, Devlin wrote: > I understand the motivation for this, but if/when we move to promise-based apis, > this will make the transition harder. Let's just create the result in all > cases. Extensions shouldn't be calling discard very frequently. Done. https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/2153943002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.h:325: class TabsDiscardFunction : public ChromeSyncExtensionFunction { On 2016/07/19 22:49:12, Devlin wrote: > ChromeSyncExtensionFunction is dead. Prefer UIThreadExtensionFunction. Done. https://codereview.chromium.org/2153943002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2153943002/diff/80001/chrome/common/extension... chrome/common/extensions/api/tabs.json:914: "description": "Discards a tab from memory. Discarded tabs are still visible on the tab strip and get reloaded when clicked on.", On 2016/07/19 22:49:12, Devlin wrote: > s/clicked on/activated. Done. https://codereview.chromium.org/2153943002/diff/80001/chrome/common/extension... chrome/common/extensions/api/tabs.json:937: "description": "Status describing if the tab was successfully discarded or not." On 2016/07/19 22:49:12, Devlin wrote: > Is there a reason for this boolean rather than using lastError? The idea is that the boolean is related to the TabManager result and not to the API function execution. It's not an error such as when a wrong tabId is passed. It means that chrome.tabs.discard() executed with no errors but no tab was discarded.
https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:77: #include "extensions/browser/extension_function.h" no need to include this, since it's included in the header. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2195: // real tab in case of success (contents is not null). Why not return undefined in the case of no tab discarded? That would also obviate the need for the extra boolean, right? https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1449: content::WebContents* web_contents = browser()->OpenURL(params); We should probably wait for load stop here. ui_test_utils::NavigateToURL() may be a better fit. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1455: discard->set_has_callback(true); Is this necessary? https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1464: discard->GetResultList()->Get(1, &result_success); Couldn't we just use ListValue::GetBoolean()? https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1469: // Confirms that TabManager sees the the tab as discarded. typo: 'the the' https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1478: discard->GetResultList()->Get(0, &result_tab); here, too - why not just GetDictionary? https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1496: content::WebContents* web_contents = browser()->OpenURL(params); here too, prefer NavigateToURL so that we know the notification is received by the proper source. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1514: // Make sure the result was successfull. s/successfull/successful https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/tabs.json:914: "description": "Discards a tab from memory. Discarded tabs are still visible on the tab strip and get reloaded when activated.", nit: s/get reloaded/are reloaded I also wonder if the "reloaded" terminology here is a little ambiguous, because we page the tab back into memory rather than refreshing the page. Is there another canonical way of saying this? If not, this probably isn't that bad. https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/tabs.json:932: "description": "Discarded tab if it was successfully discarded." Tying into the comment in the api.cc file, could this be an optional param, which would indicate success by its presence?
https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:77: #include "extensions/browser/extension_function.h" On 2016/07/21 15:39:21, Devlin wrote: > no need to include this, since it's included in the header. Done. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2195: // real tab in case of success (contents is not null). On 2016/07/21 15:39:21, Devlin wrote: > Why not return undefined in the case of no tab discarded? That would also > obviate the need for the extra boolean, right? Done. Removed the boolean. It was necessary before but with some design changes we did, it isn't anymore. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1449: content::WebContents* web_contents = browser()->OpenURL(params); On 2016/07/21 15:39:21, Devlin wrote: > We should probably wait for load stop here. ui_test_utils::NavigateToURL() may > be a better fit. Done. Had to use NavigateToURLWithDisposition() instead, as we are creating a new tab. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1455: discard->set_has_callback(true); On 2016/07/21 15:39:21, Devlin wrote: > Is this necessary? Not anymore, because we are always returning result. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1464: discard->GetResultList()->Get(1, &result_success); On 2016/07/21 15:39:21, Devlin wrote: > Couldn't we just use ListValue::GetBoolean()? Yes, we probably could. I didn't see we had that. Now this is much more simpler without the boolean. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1469: // Confirms that TabManager sees the the tab as discarded. On 2016/07/21 15:39:21, Devlin wrote: > typo: 'the the' Done. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1478: discard->GetResultList()->Get(0, &result_tab); On 2016/07/21 15:39:21, Devlin wrote: > here, too - why not just GetDictionary? Done. Same. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1496: content::WebContents* web_contents = browser()->OpenURL(params); On 2016/07/21 15:39:21, Devlin wrote: > here too, prefer NavigateToURL so that we know the notification is received by > the proper source. Done. https://codereview.chromium.org/2153943002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1514: // Make sure the result was successfull. On 2016/07/21 15:39:21, Devlin wrote: > s/successfull/successful Done. https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/tabs.json:914: "description": "Discards a tab from memory. Discarded tabs are still visible on the tab strip and get reloaded when activated.", On 2016/07/21 15:39:21, Devlin wrote: > nit: s/get reloaded/are reloaded > > I also wonder if the "reloaded" terminology here is a little ambiguous, because > we page the tab back into memory rather than refreshing the page. Is there > another canonical way of saying this? If not, this probably isn't that bad. Reloaded is what we've always been using, so we are using here for consistency. https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/tabs.json:932: "description": "Discarded tab if it was successfully discarded." On 2016/07/21 15:39:21, Devlin wrote: > Tying into the comment in the api.cc file, could this be an optional param, > which would indicate success by its presence? Solved by removing the boolean and by returning undefined in case of failure.
https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2153943002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/tabs.json:914: "description": "Discards a tab from memory. Discarded tabs are still visible on the tab strip and get reloaded when activated.", On 2016/07/21 20:43:04, Anderson Silva wrote: > On 2016/07/21 15:39:21, Devlin wrote: > > nit: s/get reloaded/are reloaded > > > > I also wonder if the "reloaded" terminology here is a little ambiguous, > because > > we page the tab back into memory rather than refreshing the page. Is there > > another canonical way of saying this? If not, this probably isn't that bad. > > Reloaded is what we've always been using, so we are using here for consistency. We can definitely revisit the "reload" terminology and find a better word for it, but consistency for now is good. Just a quick comment that the tab does somewhat get refreshed, albeit mostly from cache when possible. But sites such as GMail get reloaded from network entirely. If you have suggestions, we're all ears! :)
https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2195: std::unique_ptr<tabs::Tab> tab(new tabs::Tab); This won't be undefined, then, it'll be a tab object with default properties. This also implies that there's a test missing (passing in an invalid tab id). ;)
https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2195: std::unique_ptr<tabs::Tab> tab(new tabs::Tab); On 2016/07/22 18:39:19, Devlin wrote: > This won't be undefined, then, it'll be a tab object with default properties. > > This also implies that there's a test missing (passing in an invalid tab id). ;) Yes, it's a tab object. I tested in an extension and the fields of the object are undefined though. I'm trying to return the tab as undefined but can't get it working or find an example. If I don't return anything, it throws an exception because the Tab is required. Tests depend on this so I'm hold. Do you know if this is feasible? Or should we change function description (in the json) setting the returned Tab as optional? Thanks
https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2195: std::unique_ptr<tabs::Tab> tab(new tabs::Tab); On 2016/07/22 20:18:25, Anderson Silva wrote: > Do you know if this is feasible? Or should we change function description (in > the json) setting the returned Tab as optional? ^^ This. :) Marking the field as optional and then using an empty unique_ptr should do the trick, I think.
https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2195: std::unique_ptr<tabs::Tab> tab(new tabs::Tab); On 2016/07/22 20:20:53, Devlin wrote: > On 2016/07/22 20:18:25, Anderson Silva wrote: > > Do you know if this is feasible? Or should we change function description (in > > the json) setting the returned Tab as optional? > ^^ This. :) Marking the field as optional and then using an empty unique_ptr > should do the trick, I think. Done. Empty unique_ptr crashes but no arguments works just fine. Added two new tests for this. One that catches the error when passed invalid id and another that makes sure the result is undefined when no tab is discarded.
Also, do we intend to have the ability to un-discard a tab programmatically? The design doc wasn't clear on this point. https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2197: } else { nit: no need for an else {} after a return. https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1490: 2; This seems fragile. We should assert that there isn't a tab with this id. https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1549: // function execution exits with no errors but the tab couldn't be This still seems like it *should* be an error to me. If I'm an extension developer, what am I supposed to do with this information? I have no idea why the tab didn't correctly discard, only that it didn't.
About reloading, there's already a chrome.tabs.reload() function in the API that'll reload the tab. Also, if the user interact with it, chrome internal system will detect activity and reload the tab. So we don't need to add any changes for that. We'll update the document to contain this information. https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2197: } else { On 2016/07/25 15:48:55, Devlin wrote: > nit: no need for an else {} after a return. Done. https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1490: 2; On 2016/07/25 15:48:55, Devlin wrote: > This seems fragile. We should assert that there isn't a tab with this id. Done. https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1549: // function execution exits with no errors but the tab couldn't be On 2016/07/25 15:48:55, Devlin wrote: > This still seems like it *should* be an error to me. If I'm an extension > developer, what am I supposed to do with this information? I have no idea why > the tab didn't correctly discard, only that it didn't. Yes, this is by design. It's more of a "best effort", as we'll try to discard when asked to, but we don't guarantee that it will succeed. Depending on whether a tab is supplied or not: - If not tab is supplied, this will fail if there are no discardable tabs currently. This depends on multiple signals (active, playing audio...) that we are not exposing. - If a tab is supplied, discarding will fail if the tab is active or if it's already discarded. I changed the description of the tabId parameter to better reflect this. Let me know what you think.
On 2016/07/25 17:42:00, Anderson Silva wrote: > About reloading, there's already a chrome.tabs.reload() function in the API > that'll reload the tab. Also, if the user interact with it, chrome internal > system will detect activity and reload the tab. So we don't need to add any > changes for that. Now we're back to "reload" being overloaded. :) AFAIK, the tabs.reload() method does a typical reload - like what would happen if the user hit f5. But if there's, for instance, form content, this form content is typically lost. With discarded tabs, if Chrome reloads the discarded tab because the user interacts with it, it restores that form content. Would this happen if the extension called tabs.reload()? https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1549: // function execution exits with no errors but the tab couldn't be On 2016/07/25 17:42:00, Anderson Silva wrote: > On 2016/07/25 15:48:55, Devlin wrote: > > This still seems like it *should* be an error to me. If I'm an extension > > developer, what am I supposed to do with this information? I have no idea why > > the tab didn't correctly discard, only that it didn't. > > Yes, this is by design. It's more of a "best effort", as we'll try to discard > when asked to, but we don't guarantee that it will succeed. > > Depending on whether a tab is supplied or not: > - If not tab is supplied, this will fail if there are no discardable tabs > currently. This depends on multiple signals (active, playing audio...) that we > are not exposing. > - If a tab is supplied, discarding will fail if the tab is active or if it's > already discarded. > > I changed the description of the tabId parameter to better reflect this. > > Let me know what you think. I'm fine with the best-effort approach and with not always being able to discard a tab, but I think we should let the developer know why something failed. Was nothing discarded because there were no discardable tabs? Because it was too soon to discard the specified tab? Because the tab was not discardable? These are things that developers may need to rely on, and right now we're being totally opaque about them. Is there a reason not to provide the reason that the call failed?
On 2016/07/25 18:05:44, Devlin wrote: > On 2016/07/25 17:42:00, Anderson Silva wrote: > > About reloading, there's already a chrome.tabs.reload() function in the API > > that'll reload the tab. Also, if the user interact with it, chrome internal > > system will detect activity and reload the tab. So we don't need to add any > > changes for that. > > Now we're back to "reload" being overloaded. :) AFAIK, the tabs.reload() method > does a typical reload - like what would happen if the user hit f5. But if > there's, for instance, form content, this form content is typically lost. With > discarded tabs, if Chrome reloads the discarded tab because the user interacts > with it, it restores that form content. Would this happen if the extension > called tabs.reload()? Whether the reload happens because of the tab activation or being explicitly reloaded (context menu), the content does get restored. The magic is actually in the discard itself, it makes sure that the for content gets restored at next reload (regardless of how it was reloaded). We double checked that tabs.reload does the same thing and it works :) That's why there's no point in adding an "undiscard" function, as it would just call reload internally.. > > https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... > chrome/browser/extensions/api/tabs/tabs_test.cc:1549: // function execution > exits with no errors but the tab couldn't be > On 2016/07/25 17:42:00, Anderson Silva wrote: > > On 2016/07/25 15:48:55, Devlin wrote: > > > This still seems like it *should* be an error to me. If I'm an extension > > > developer, what am I supposed to do with this information? I have no idea > why > > > the tab didn't correctly discard, only that it didn't. > > > > Yes, this is by design. It's more of a "best effort", as we'll try to discard > > when asked to, but we don't guarantee that it will succeed. > > > > Depending on whether a tab is supplied or not: > > - If not tab is supplied, this will fail if there are no discardable tabs > > currently. This depends on multiple signals (active, playing audio...) that we > > are not exposing. > > - If a tab is supplied, discarding will fail if the tab is active or if it's > > already discarded. > > > > I changed the description of the tabId parameter to better reflect this. > > > > Let me know what you think. > > I'm fine with the best-effort approach and with not always being able to discard > a tab, but I think we should let the developer know why something failed. Was > nothing discarded because there were no discardable tabs? Because it was too > soon to discard the specified tab? Because the tab was not discardable? These > are things that developers may need to rely on, and right now we're being > totally opaque about them. Is there a reason not to provide the reason that the > call failed? When discarding a specific tab, that can only fail if it's activated or already discarded (both are easy for extensions to check for). When asking the browser to discard a tab based on the internal ranking, the reason for failure might change with time, while we keep tweaking the algorithm. As we want the API to remain stable, we are intentionally not exposing all those checks to the extensions. And they can't override/tweak them either. The way we see it is that extensions that wish to take over completely the discarding features will never use that function without specifying a tab.
On 2016/07/25 19:49:37, Georges Khalil wrote: > On 2016/07/25 18:05:44, Devlin wrote: > > On 2016/07/25 17:42:00, Anderson Silva wrote: > > > About reloading, there's already a chrome.tabs.reload() function in the API > > > that'll reload the tab. Also, if the user interact with it, chrome internal > > > system will detect activity and reload the tab. So we don't need to add any > > > changes for that. > > > > Now we're back to "reload" being overloaded. :) AFAIK, the tabs.reload() > method > > does a typical reload - like what would happen if the user hit f5. But if > > there's, for instance, form content, this form content is typically lost. > With > > discarded tabs, if Chrome reloads the discarded tab because the user interacts > > with it, it restores that form content. Would this happen if the extension > > called tabs.reload()? > > Whether the reload happens because of the tab activation or being explicitly > reloaded (context menu), the content does get restored. The magic is actually in > the discard itself, it makes sure that the for content gets restored at next > reload (regardless of how it was reloaded). > > We double checked that tabs.reload does the same thing and it works :) That's > why there's no point in adding an "undiscard" function, as it would just call > reload internally.. > > > > > > https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... > > File chrome/browser/extensions/api/tabs/tabs_test.cc (right): > > > > > https://codereview.chromium.org/2153943002/diff/160001/chrome/browser/extensi... > > chrome/browser/extensions/api/tabs/tabs_test.cc:1549: // function execution > > exits with no errors but the tab couldn't be > > On 2016/07/25 17:42:00, Anderson Silva wrote: > > > On 2016/07/25 15:48:55, Devlin wrote: > > > > This still seems like it *should* be an error to me. If I'm an extension > > > > developer, what am I supposed to do with this information? I have no idea > > why > > > > the tab didn't correctly discard, only that it didn't. > > > > > > Yes, this is by design. It's more of a "best effort", as we'll try to > discard > > > when asked to, but we don't guarantee that it will succeed. > > > > > > Depending on whether a tab is supplied or not: > > > - If not tab is supplied, this will fail if there are no discardable tabs > > > currently. This depends on multiple signals (active, playing audio...) that > we > > > are not exposing. > > > - If a tab is supplied, discarding will fail if the tab is active or if it's > > > already discarded. > > > > > > I changed the description of the tabId parameter to better reflect this. > > > > > > Let me know what you think. > > > > I'm fine with the best-effort approach and with not always being able to > discard > > a tab, but I think we should let the developer know why something failed. Was > > nothing discarded because there were no discardable tabs? Because it was too > > soon to discard the specified tab? Because the tab was not discardable? > These > > are things that developers may need to rely on, and right now we're being > > totally opaque about them. Is there a reason not to provide the reason that > the > > call failed? > > When discarding a specific tab, that can only fail if it's activated or already > discarded (both are easy for extensions to check for). When asking the browser > to discard a tab based on the internal ranking, the reason for failure might > change with time, while we keep tweaking the algorithm. As we want the API to > remain stable, we are intentionally not exposing all those checks to the > extensions. And they can't override/tweak them either. The way we see it is that > extensions that wish to take over completely the discarding features will never > use that function without specifying a tab. Trying this again as no email was sent for some reason.
On 2016/07/25 21:10:35, Georges Khalil wrote: > > Whether the reload happens because of the tab activation or being explicitly > > reloaded (context menu), the content does get restored. The magic is actually > in > > the discard itself, it makes sure that the for content gets restored at next > > reload (regardless of how it was reloaded). > > > > We double checked that tabs.reload does the same thing and it works :) That's > > why there's no point in adding an "undiscard" function, as it would just call > > reload internally.. Okay, nifty. We should mention that in the documentation, but otherwise, sounds good! > > When discarding a specific tab, that can only fail if it's activated or > already > > discarded (both are easy for extensions to check for). Can't it also fail if it hasn't been long enough (minimum_protection_time)? And theoretically, yes, the extension could check for these criteria, but that means the extension needs to know all these criteria beforehand. The point of the error is to inform the developer that something failed, which can often be something they could theoretically have avoided, but it makes for a much better experience if we just let them know what went wrong. For instance, we throw errors if no tab with a specified id is found, even though it's also easy for extensions to check the existence of a tab with the given id. It's much better to be clear than to transparently do nothing and leave the developer to delve into complex chrome code to figure out why it might not have worked. :) > > When asking the browser > > to discard a tab based on the internal ranking, the reason for failure might > > change with time, while we keep tweaking the algorithm. As we want the API to > > remain stable, we are intentionally not exposing all those checks to the > > extensions. And they can't override/tweak them either. The way we see it is > that > > extensions that wish to take over completely the discarding features will > never > > use that function without specifying a tab. I'm not suggesting we expose any/all complex tab discarding picking to the extension. But we could easily say "Could not find a tab to discard", which explains why we didn't discard anything while leaving us the flexibility to tweak exactly when that might happen. But then at least the developer knows it was intentionally not discarded, and has some inkling of the issue. In short, I'd basically recommend a small set of errors for the following scenarios: - No tab with the given id (already there) - Cannot discard tab with given id - Up to you if you want to make this more specific and include e.g. "tab is active", "tab hasn't been idle long enough", etc. - Cannot find a tab to discard (with no id specified)
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...
On 2016/07/25 21:23:04, Devlin wrote: > On 2016/07/25 21:10:35, Georges Khalil wrote: > > > Whether the reload happens because of the tab activation or being explicitly > > > reloaded (context menu), the content does get restored. The magic is > actually > > in > > > the discard itself, it makes sure that the for content gets restored at next > > > reload (regardless of how it was reloaded). > > > > > > We double checked that tabs.reload does the same thing and it works :) > That's > > > why there's no point in adding an "undiscard" function, as it would just > call > > > reload internally.. > > Okay, nifty. We should mention that in the documentation, but otherwise, sounds > good! Great. We'll update the doc with all changes we've made so far. > > > > When discarding a specific tab, that can only fail if it's activated or > > already > > > discarded (both are easy for extensions to check for). > > Can't it also fail if it hasn't been long enough (minimum_protection_time)? And > theoretically, yes, the extension could check for these criteria, but that means > the extension needs to know all these criteria beforehand. The point of the > error is to inform the developer that something failed, which can often be > something they could theoretically have avoided, but it makes for a much better > experience if we just let them know what went wrong. For instance, we throw > errors if no tab with a specified id is found, even though it's also easy for > extensions to check the existence of a tab with the given id. It's much better > to be clear than to transparently do nothing and leave the developer to delve > into complex chrome code to figure out why it might not have worked. :) > minimum_protection_time and other checks are made only when a tab is not given. In short, when a tab id is given we go there and discard it if it's not unfeasible (active or already discarded). > > > When asking the browser > > > to discard a tab based on the internal ranking, the reason for failure might > > > change with time, while we keep tweaking the algorithm. As we want the API > to > > > remain stable, we are intentionally not exposing all those checks to the > > > extensions. And they can't override/tweak them either. The way we see it is > > that > > > extensions that wish to take over completely the discarding features will > > never > > > use that function without specifying a tab. > > I'm not suggesting we expose any/all complex tab discarding picking to the > extension. But we could easily say "Could not find a tab to discard", which > explains why we didn't discard anything while leaving us the flexibility to > tweak exactly when that might happen. But then at least the developer knows it > was intentionally not discarded, and has some inkling of the issue. > > In short, I'd basically recommend a small set of errors for the following > scenarios: > - No tab with the given id (already there) > - Cannot discard tab with given id > - Up to you if you want to make this more specific and include e.g. "tab is > active", "tab hasn't been idle long enough", etc. > - Cannot find a tab to discard (with no id specified) Good. We were worried about having to expose and tell about the internal system but if that isn't the case the error messages are fine. It'll help more the developers. I've included the messages and a new test in the last patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Nice! LGTM with a few last nits https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2200: error_ = ErrorUtils::FormatErrorMessage(keys::kCannotDiscardTab, nit: setting error_ is handled by returning an Error() ResponseAction, so I'd just inline these: return RespondNow(Error( params->tab_id ? ErrorUtils::FormatErrorMessage(keys::kCannotDiscardTab, base::IntToString(*params->tab_id)) : kCannotFindTabToDiscard)); https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1468: // Make sure the returned tab is the one discarded and nit: line wrapping looks off here? https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1476: IN_PROC_BROWSER_TEST_F(ExtensionTabsTest, DiscardTabAlreadyDiscarded) { We could probably tack this on to the previous test, since the tab will be discarded on line 1472. https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1561: // Make sure the returned tab is the one discarded and wrapping looks off here, too https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1580: // Run without passing an id. In this case the tab and here.
Description was changed from ========== Implementing TabManager extensions API Discard Function. BUG=621070 ========== to ========== Implementing TabManager extensions API Discard Function. BUG=621070 ==========
andersoncss@google.com changed reviewers: + asvitkine@chromium.org
andersoncss@google.com changed reviewers: - asvitkine@chromium.org
andersoncss@google.com changed reviewers: + asvitkine@chromium.org
Hi Alexei, Can you please take a look at this CL? We are adding a new function to Chrome Tabs Extension API and need review from an owner of the extensions/browser/extension_function_histogram_value.h file. Thanks https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:2200: error_ = ErrorUtils::FormatErrorMessage(keys::kCannotDiscardTab, On 2016/07/26 16:13:45, Devlin wrote: > nit: setting error_ is handled by returning an Error() ResponseAction, so I'd > just inline these: > > return RespondNow(Error( > params->tab_id ? > ErrorUtils::FormatErrorMessage(keys::kCannotDiscardTab, > base::IntToString(*params->tab_id)) : > kCannotFindTabToDiscard)); > Done. https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1468: // Make sure the returned tab is the one discarded and On 2016/07/26 16:13:45, Devlin wrote: > nit: line wrapping looks off here? Done. https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1476: IN_PROC_BROWSER_TEST_F(ExtensionTabsTest, DiscardTabAlreadyDiscarded) { On 2016/07/26 16:13:45, Devlin wrote: > We could probably tack this on to the previous test, since the tab will be > discarded on line 1472. Done. https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1561: // Make sure the returned tab is the one discarded and On 2016/07/26 16:13:45, Devlin wrote: > wrapping looks off here, too Done. https://codereview.chromium.org/2153943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1580: // Run without passing an id. In this case the tab On 2016/07/26 16:13:45, Devlin wrote: > and here. 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...
lgtm
andersoncss@google.com changed reviewers: + jochen@chromium.org
Hi jochen, Can you please take a look at this CL? We are adding a new function to Chrome Tabs Extension API and need review from an owner of the tools/metrics/histograms/histograms.xml file. Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I'm only reviewing UseCounter changes, sorry. Please pick a reviewer from https://cs.chromium.org/chromium/src/tools/metrics/OWNERS
andersoncss@google.com changed reviewers: - jochen@chromium.org
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...
On 2016/07/27 07:37:11, jochen wrote: > I'm only reviewing UseCounter changes, sorry. Please pick a reviewer from > https://cs.chromium.org/chromium/src/tools/metrics/OWNERS FYI: your already have my lgtm for histograms here
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andersoncss@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from georgesak@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2153943002/#ps260001 (title: "reset protection time")
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 ========== Implementing TabManager extensions API Discard Function. BUG=621070 ========== to ========== Implementing TabManager extensions API Discard Function. BUG=621070 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Implementing TabManager extensions API Discard Function. BUG=621070 ========== to ========== Implementing TabManager extensions API Discard Function. BUG=621070 Committed: https://crrev.com/71250fbd07f01e5f5f27bccce5ac23d4c94e3563 Cr-Commit-Position: refs/heads/master@{#408137} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/71250fbd07f01e5f5f27bccce5ac23d4c94e3563 Cr-Commit-Position: refs/heads/master@{#408137} |
