Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
987583004:
Add audible, muted to Tab, c.t.query, c.t.update, and c.t.onUpdated where relevant (Closed)
|
Created:
5 years, 9 months ago by Jared Sohn Modified:
5 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@patch1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd audible, muted to Tab, c.t.query, c.t.update, and c.t.onUpdated where relevant
chrome.tabs.update will reject the audible property.
When the onUpdated event is triggered for the muted property, it sets cause
to "user", "capture", or the extension id that caused it
Note: Requires --enable-tab-audio-muting flag to be active (chrome://flags) in order to set a tab's muted state via c.t.update.
BUG=438903
Committed: https://crrev.com/09a3ccc44eb5b7552d6ec49dc8fb46678dd4025f
Cr-Commit-Position: refs/heads/master@{#337981}
Patch Set 1 #Patch Set 2 : attempt to add kalman as reviewer via git-cl owners (didn't work) #
Total comments: 26
Patch Set 3 : Response to feedback from messages #3, #4 #Patch Set 4 : remove unneeded header files #
Total comments: 11
Patch Set 5 : Followup for review from message #10 #
Total comments: 1
Patch Set 6 : Move TabEntry forward declaration closer to method that uses it (Response to codereview message #17) #Patch Set 7 : tests; also mutedCause is a part of the tab object and capture mutedCause changed to just 'capture' #
Total comments: 49
Patch Set 8 : Followup for review of patch #7 #
Total comments: 2
Patch Set 9 : change order of definitions in TabEntry #Patch Set 10 : show message when user attempts to change mute state for tab being captured #Patch Set 11 : header file order fixed #
Total comments: 2
Patch Set 12 : followups through message #33 #
Total comments: 12
Patch Set 13 : followups through message #36 #
Total comments: 3
Patch Set 14 : Change tabIsUnmutedWhenTabCaptured test to not require a timeout #
Total comments: 1
Patch Set 15 : rebased #Patch Set 16 : Make TabsEventRouter a friend to TabEntry so that it will build on some platforms #
Total comments: 1
Patch Set 17 : TabEntry map stores entries as linked_ptrs #Patch Set 18 : Including missing html/js files #Patch Set 19 : rebase #Messages
Total messages: 93 (24 generated)
jared.sohn@gmail.com changed reviewers: + kalman@chromium.org, miu@chromium.org
Looks great! Comments: https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:72: DispatchTabUpdatedEvent(contents, changed_properties.Pass()); Instead of dispatching separate events here and in TabMutedStateChanged, can you create one changed_properties dictionary in TabUpdated()? For efficiency, all the changes can be dispatched in a single event with multiple changed properties. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:84: cause); nit: indent one less space, and run `git cl format` to fix a few other things https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:90: wasAudible_(false), A TabEntry might be constructed after a tab is alive and already playing audio (e.g., if a tab is dragged between browser windows, or TabsEventRouter was constructed late). I think this constructor should be: TabsEventRouter::TabEntry::TabEntry(content::WebContents* contents) : ..., was_audible_(contents->WasRecentlyAudible()), was_muted_(contents->IsAudioMuted()), ... { } https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:127: bool TabsEventRouter::TabEntry::AudibleChanged(bool val) const { nit: Your call, but consider combining each pair of these methods into a single method that sets the new value and returns true if the value actually changed. Example: bool TabsEventRouter::TabEntry::SetAudible(bool new_val) { if (was_audible_ == new_val) return false; was_audible_ = new_val; return true; } https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:516: nit: remove added newlines throughout this method https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:176: bool wasAudible_; style: No camel case for variables in Chromium C++: bool was_audible_; bool was_muted_;
https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:5: #include <string> not needed, header file already includes <string> https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:68: new base::DictionaryValue()); I just submitted https://codereview.chromium.org/1000443002/ which should make this easier: scoped_ptr<base::DictionaryValue> changed_properties = DictionaryBuilder() .SetBoolean(tabs_constants::kAudibleKey, audible) .Build(); etc; even nicer below. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:127: bool TabsEventRouter::TabEntry::AudibleChanged(bool val) const { On 2015/03/10 18:51:39, miu wrote: > nit: Your call, but consider combining each pair of these methods into a single > method that sets the new value and returns true if the value actually changed. > Example: > > bool TabsEventRouter::TabEntry::SetAudible(bool new_val) { > if (was_audible_ == new_val) > return false; > was_audible_ = new_val; > return true; > } +1 and also no parens around these expressions. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:144: only 1 blank line https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:460: } This code should be consistent with the existing code - unfortunately the existing code is odd and doesn't scale very well to more functionality (with that single |did_navigate| parameter). How about you change TabUpdated to take a scoped_ptr<base::DictionaryValue> instead of the contents/boolean: TabUpdated(contents, true) --> TabUpdated(GetTabEntry(contents)->DidNavigate()); and likewise TabUpdate(contents, false) --> TabUpdated(GetTabEntry(contents)->UpdateLoadState()); That would require TabEntry to hold onto its WebContents, but that seems like the right thing to do anyway. https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/tabs.json:23: "audible": {"type": "boolean", "optional": true, "description": "Whether the tab is audible."}, Explain the difference between audible and muted, here and below. https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/tabs.json:891: "cause": { I'd rather "mutedCause" because this is muted specific.
Responses for Patch 2 below. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:5: #include <string> On 2015/03/10 22:30:08, kalman wrote: > not needed, header file already includes <string> I added that to remove a lint error: (build/include_what_you_use) Do you still want it removed? (The lint message suggests that it should go in the header or the source file, so at the very least it would make more sense to go in the header file since the method signatures make use of it.) https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:72: DispatchTabUpdatedEvent(contents, changed_properties.Pass()); On 2015/03/10 at 18:51:39, miu wrote: > Instead of dispatching separate events here and in TabMutedStateChanged, can you create one changed_properties dictionary in TabUpdated()? For efficiency, all the changes can be dispatched in a single event with multiple changed properties. Done. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:84: cause); On 2015/03/10 18:51:39, miu wrote: > nit: indent one less space, and run `git cl format` to fix a few other things Done. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:127: bool TabsEventRouter::TabEntry::AudibleChanged(bool val) const { On 2015/03/10 at 22:30:08, kalman wrote: > On 2015/03/10 18:51:39, miu wrote: > > nit: Your call, but consider combining each pair of these methods into a single > > method that sets the new value and returns true if the value actually changed. > > Example: > > > > bool TabsEventRouter::TabEntry::SetAudible(bool new_val) { > > if (was_audible_ == new_val) > > return false; > > was_audible_ = new_val; > > return true; > > } > > +1 and also no parens around these expressions. Done. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:127: bool TabsEventRouter::TabEntry::AudibleChanged(bool val) const { On 2015/03/10 18:51:39, miu wrote: > nit: Your call, but consider combining each pair of these methods into a single > method that sets the new value and returns true if the value actually changed. > Example: > > bool TabsEventRouter::TabEntry::SetAudible(bool new_val) { > if (was_audible_ == new_val) > return false; > was_audible_ = new_val; > return true; > } Done. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:144: On 2015/03/10 at 22:30:08, kalman wrote: > only 1 blank line Done. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:460: } On 2015/03/10 at 22:30:08, kalman wrote: > This code should be consistent with the existing code - unfortunately the existing code is odd and doesn't scale very well to more functionality (with that single |did_navigate| parameter). > > How about you change TabUpdated to take a scoped_ptr<base::DictionaryValue> instead of the contents/boolean: > > TabUpdated(contents, true) --> > TabUpdated(GetTabEntry(contents)->DidNavigate()); > > and likewise > > TabUpdate(contents, false) --> > TabUpdated(GetTabEntry(contents)->UpdateLoadState()); > > That would require TabEntry to hold onto its WebContents, but that seems like the right thing to do anyway. At the moment, I am choosing to pass an extensions::DictionaryBuilder object instead of a scoped_ptr<base::DictionaryValue>. This simplifies the code; is there a reason not to pass it instead? (Or maybe you wrote this comment before you exposed DictionaryBuilders to extensions?) I have updated the code to include the WebContents as a part of the TabEntry. Unfortunately, I don't think we can remove the need for passing in a WebContents object here since we get the TabEntry from that. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:516: On 2015/03/10 18:51:39, miu wrote: > nit: remove added newlines throughout this method Done. https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:176: bool wasAudible_; On 2015/03/10 18:51:39, miu wrote: > style: No camel case for variables in Chromium C++: > > bool was_audible_; > bool was_muted_; Done. https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/tabs.json:23: "audible": {"type": "boolean", "optional": true, "description": "Whether the tab is audible."}, On 2015/03/10 at 22:30:08, kalman wrote: > Explain the difference between audible and muted, here and below. Here is my new verbiage (feel free to give feedback for improving it.): audible: Whether the tab has produced sound over the past couple of seconds (but it might not be heard if also muted). Equivalent to whether the speaker audio indicator is showing. muted: Whether the tab is prevented from playing sound (but hasn't necessarily recently produced sound). Equivalent to whether the muted audio indicator is showing. For now I have not altered the text for the other audible/muted parameters since this way seems more consistent with the documentation for other properties such as selected, highlighted, active, and pinned. https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/tabs.json:891: "cause": { On 2015/03/10 at 22:30:08, kalman wrote: > I'd rather "mutedCause" because this is muted specific. I had been debating that; "cause" could be reused if also needed for another onUpdated message, but perhaps the extra specificity makes this easier to think about and also makes it more googleable. Change made here and throughout the rest of the patch.
extensions/common/value_builder.h and extensions/extensions.gypi will be removed from the patch (they are here to add DictionaryBuilder support. I am not able to build right now due to linking errors re: DictionaryBuilder; I suspect it is because it isn't applying the new GYP. What command do I need to use to do that? Also, I modified DictionaryBuilder to add a size() method; I suspect this will also be removed from this patch.
On 2015/03/16 at 02:42:03, Jared Sohn wrote: > extensions/common/value_builder.h and extensions/extensions.gypi will be removed from the patch (they are here to add DictionaryBuilder support. I am not able to build right now due to linking errors re: DictionaryBuilder; I suspect it is because it isn't applying the new GYP. What command do I need to use to do that? > > Also, I modified DictionaryBuilder to add a size() method; I suspect this will also be removed from this patch. I was a little confused; the DictionaryBuilder patch also affected input_method_api.cc. value_builder.h only includes my addition of the size() method. Regardless, I suspect none of these files will be a part of the patch.
>What command do I need to use to do that? Nevermind; git-cl runhooks does it and I can build now.
https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:460: } On 2015/03/16 02:34:29, Jared Sohn wrote: > On 2015/03/10 at 22:30:08, kalman wrote: > > This code should be consistent with the existing code - unfortunately the > existing code is odd and doesn't scale very well to more functionality (with > that single |did_navigate| parameter). > > > > How about you change TabUpdated to take a scoped_ptr<base::DictionaryValue> > instead of the contents/boolean: > > > > TabUpdated(contents, true) --> > > TabUpdated(GetTabEntry(contents)->DidNavigate()); > > > > and likewise > > > > TabUpdate(contents, false) --> > > TabUpdated(GetTabEntry(contents)->UpdateLoadState()); > > > > That would require TabEntry to hold onto its WebContents, but that seems like > the right thing to do anyway. > > At the moment, I am choosing to pass an extensions::DictionaryBuilder object > instead of a scoped_ptr<base::DictionaryValue>. This simplifies the code; is > there a reason not to pass it instead? (Or maybe you wrote this comment before > you exposed DictionaryBuilders to extensions?) > > I have updated the code to include the WebContents as a part of the TabEntry. > Unfortunately, I don't think we can remove the need for passing in a WebContents > object here since we get the TabEntry from that. Passing a DictionaryBuilder is fine. I hadn't thought of that. https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/987583004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/tabs.json:23: "audible": {"type": "boolean", "optional": true, "description": "Whether the tab is audible."}, On 2015/03/16 02:34:29, Jared Sohn wrote: > On 2015/03/10 at 22:30:08, kalman wrote: > > Explain the difference between audible and muted, here and below. > > Here is my new verbiage (feel free to give feedback for improving it.): > > audible: Whether the tab has produced sound over the past couple of seconds (but > it might not be heard if also muted). Equivalent to whether the speaker audio > indicator is showing. Perfect. > > muted: Whether the tab is prevented from playing sound (but hasn't necessarily > recently produced sound). Equivalent to whether the muted audio indicator is > showing. Perfect. > > For now I have not altered the text for the other audible/muted parameters since > this way seems more consistent with the documentation for other properties such > as selected, highlighted, active, and pinned. Thanks.
Is there a way you can test these changes? - preferably unit test - if not, and probably necessarily, an API test (see how the existing tabs API tests work - https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... ) https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:67: url_() { It's not necessary to initialize url() https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:91: return changed_properties; Builder lets you do: return DictionaryValue() .Set(tabs_contents::kStatusKey, tabs_constants::kStatusValueComplete)) .Release(); Same elsewhere. As I mentioned in the .h file, if you're not chaining together these calls there's no point using DictionaryBuilder, it's pure syntactic sugar. https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:420: DictionaryBuilder* changed_properties) { Yeah, I think that passing a DictionaryValue in here is better (and a scoped_ptr<DictionaryValue> at that). https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.cc:435: if (changed_properties->size() > 0) { use .empty() preferably. https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:36: class TabEntry; Declare this above TabUpdated. Odd having a private: section up the top. Though really the whole class definition of TabEntry should be the first thing in the private: block, making the forward declaration unnecessary, but whatever. Don't do that, unnecessary churn :-) https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:149: // if the state changed, false otherwise. Whether the state has changed or Comment needs to be rewritten, this doesn't return true/false https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:154: DictionaryBuilder* UpdateLoadState(); Better to pass a scoped_ptr<DictionaryBuilder> here (and from DidNavigate) to indicate that ownership is passed out of this function. Unfortunately this is a little bit awkward with the current design of DictionaryValue - the whole point of DictionaryBuilder is so that you can chain together constructions, like I mentioned in the .cc file. You might as well just use DictionaryValue otherwise. However. the .Set() methods return a DictionaryValue& not DictionaryValue*. Proposal: add a .Release() method to DictionaryBuilder which returns a scoped_ptr to itself. You can then write the code I mentioned in the .cc file. *However*, all that said - perhaps we should just be returning DictionaryValue (well, scoped_ptr<DictionaryValue>). That seems more correct to me and avoids this awkwardness altogether. https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:165: // TODO(jaredsohn): make it private or add public accessor Please do this now - public web_contents() accessor SGTM. https://codereview.chromium.org/987583004/diff/60001/extensions/common/value_... File extensions/common/value_builder.h (right): https://codereview.chromium.org/987583004/diff/60001/extensions/common/value_... extensions/common/value_builder.h:60: long size() const { return dict_->size(); } size_t but this change might not be necessary https://codereview.chromium.org/987583004/diff/60001/extensions/extensions.gypi File extensions/extensions.gypi (right): https://codereview.chromium.org/987583004/diff/60001/extensions/extensions.gy... extensions/extensions.gypi:213: 'common/value_builder.cc', Rebase and you should pick up these changes.
On 2015/03/16 18:11:56, kalman wrote: > Is there a way you can test these changes? > - if not, and probably necessarily, an API test (see how the existing tabs API > tests work - > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > ) Jared, since you're new to this: The API tests consist of C++ code and JavaScript/HTML code (the latter in src/chrome/test/data/extensions/api_test/tabs). The C++ code sets up a browser that executes the JavaScript as if it were an extension. I think you could just add something to this file to test the changes to the onUpdated() event handler: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... In order to get a tab to emit audio, an easy way is to use the JavaScript WebAudio API. Lines 54-75 here are a good example of how to emit a sine wave: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... One test might do something like: 1. Check that chrome.tabs.query returns audible==false. 2. Start the sine wave audio. 3. Expect that the onUpdated() event handler is eventually called. 4. Check that chrome.tabs.query now returns audible==true. 5. Stop the sine wave. 6. #3 and #4 for audible==false again. And another test, roughly similar to the above, for the mute setting and querying the mute state. Also, the C++ part of the test will have to modify the command line to include the --enable-tab-audio-muting flag (switches::kEnableTabAudioMuting). An example of how to do that: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> *However*, all that said - perhaps we should just be returning DictionaryValue (well, scoped_ptr<DictionaryValue>). That seems more correct to me and avoids this awkwardness altogether. > I'm removing the use of DictionaryBuilder; as you said there really isn't a great benefit for using it here (I think this is primarily because the dictionary we build requires conditional logic and this logic spans multiple methods.) Posting a new patch shortly; all issues that I am aware of have been addressed (I'm leaving TabEntry's forward declaration alone), other than adding new testing.
Have you posted the new patch?
On 2015/03/19 at 20:54:59, kalman wrote: > Have you posted the new patch? Yeah; patch 5 above.
code lg, any luck with tests? https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:36: class TabEntry; On 2015/03/16 18:11:56, kalman wrote: > Declare this above TabUpdated. Odd having a private: section up the top. > > Though really the whole class definition of TabEntry should be the first thing > in the private: block, making the forward declaration unnecessary, but whatever. > Don't do that, unnecessary churn :-) Not addressed? https://codereview.chromium.org/987583004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_event_router.h:35: class TabEntry; (as I mentioned: please move this to the private section below, not here)
> code lg, any luck with tests? I think the tests look straightforward, but I need to work on some other things for a bit right now (will have a patch with tests by the end of the weekend at the latest). Everything I have manually tested so far is working. On 2015/03/19 at 21:23:35, kalman wrote: > > https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... > File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): > > https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... > chrome/browser/extensions/api/tabs/tabs_event_router.h:36: class TabEntry; > On 2015/03/16 18:11:56, kalman wrote: > > Declare this above TabUpdated. Odd having a private: section up the top. > > > > Though really the whole class definition of TabEntry should be the first thing > > in the private: block, making the forward declaration unnecessary, but whatever. > > Don't do that, unnecessary churn :-) > > Not addressed? I misread your "don't do that". Wouldn't it be better to move the TabUpdated method next to GetTabEntry? (or move GetTabEntry up?) Or just move the whole inner class up like you originally stated (I think the churn among all four options is equivalent and that the last option is "most right").
On 2015/03/19 21:43:39, Jared Sohn wrote: > > code lg, any luck with tests? > > I think the tests look straightforward, but I need to work on some other things > for a bit right now (will have a patch with tests by the end of the weekend at > the latest). Everything I have manually tested so far is working. > > On 2015/03/19 at 21:23:35, kalman wrote: > > > > > https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... > > File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): > > > > > https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... > > chrome/browser/extensions/api/tabs/tabs_event_router.h:36: class TabEntry; > > On 2015/03/16 18:11:56, kalman wrote: > > > Declare this above TabUpdated. Odd having a private: section up the top. > > > > > > Though really the whole class definition of TabEntry should be the first > thing > > > in the private: block, making the forward declaration unnecessary, but > whatever. > > > Don't do that, unnecessary churn :-) > > > > Not addressed? > > I misread your "don't do that". Wouldn't it be better to move the TabUpdated > method next to GetTabEntry? (or move GetTabEntry up?) Or just move the whole > inner class up like you originally stated (I think the churn among all four > options is equivalent and that the last option is "most right"). You can just move this to line 88 or whatever, and job done?
On 2015/03/19 at 21:45:08, kalman wrote: > On 2015/03/19 21:43:39, Jared Sohn wrote: > > > code lg, any luck with tests? > > > > I think the tests look straightforward, but I need to work on some other things > > for a bit right now (will have a patch with tests by the end of the weekend at > > the latest). Everything I have manually tested so far is working. > > > > On 2015/03/19 at 21:23:35, kalman wrote: > > > > > > > > https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... > > > File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): > > > > > > > > https://codereview.chromium.org/987583004/diff/60001/chrome/browser/extension... > > > chrome/browser/extensions/api/tabs/tabs_event_router.h:36: class TabEntry; > > > On 2015/03/16 18:11:56, kalman wrote: > > > > Declare this above TabUpdated. Odd having a private: section up the top. > > > > > > > > Though really the whole class definition of TabEntry should be the first > > thing > > > > in the private: block, making the forward declaration unnecessary, but > > whatever. > > > > Don't do that, unnecessary churn :-) > > > > > > Not addressed? > > > > I misread your "don't do that". Wouldn't it be better to move the TabUpdated > > method next to GetTabEntry? (or move GetTabEntry up?) Or just move the whole > > inner class up like you originally stated (I think the churn among all four > > options is equivalent and that the last option is "most right"). > > You can just move this to line 88 or whatever, and job done? Done.
Oops totally forgot about this CL! I'll get back to is ASAP.
https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:140: TabEntry(); nit: Can you move this zero-arg constructor to the "private:" section below, then add: friend class std::map<int, TabEntry>; This would allow the compiler to strictly enforce that the zero-arg constructor is only being used for the std::map<>'s default allocator. ;-) https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:142: // Allow creating a TabEntry via a webcontents. Used when moving This comment isn't quite accurate since the implementation isn't all about tracking tabs moving between windows. Instead, how about: // Create a TabEntry associated with, and tracking state changes to, |contents|. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:101: function tabisUnmutedWhenTabCaptured() { nit: Capitalize the 'i' in 'is'. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:111: (changeInfo["mutedCause"] === expectedMutedCause) && nit: This line and the next should be two more spaces to the right. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:20: testTabId_ = tab.id; kalman@'s more familiar with the JS extension testing than I am. But, IIRC, you should be calling chrome.test.succeed() for each function passed to chrome.test.runTests(). https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:32: function audibleUpdateAttempt() { naming nit: How about audibleUpdateAttemptShouldFail()? https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:53: function testStaysAudibleAfterChangingWindow() { Not sure if it's necessary to test the state after a tab is moved between windows, but I'm not against keeping this in the tests either.
Tests looking great. https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc:44: command_line->AppendSwitch(::switches::kEnableTabAudioMuting); What does this flag actually control? E.g. if the call "chrome::SetTabAudioMuted" fails if it's off, then we should include as part of the API that it fails, rather than silently failing. I'll comment in the place. https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1200: bool muted = *params->update_properties.muted; Could you make this (and I may have the names wrong here, and git cl format may choose to change it): const char* switch = base::Comment::switches::kEnableTabAudioMuting; if (base::CommandLine::HasSwitch(switch)) { chrome::SetTabAudioMuted(contents, *params->update_properties.muted, extension()->id()); } else { WriteToConsole( WARNING, base::StringPrintf("Failed to update mute state, --%s must be enabled", switch)); } https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:110: if ((typeof changeInfo.mutedCause !== "undefined") && can also use changeInfo.hasOwnProperty('mutedCause') or just forget about it altogether, since you're comparing to |expectedMutedCause| which is a string, so won't be undefined. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:112: (changeInfo["muted"] == expectedMuted)) Also to the above: not much point using === when comparing to the string "capture", but arguably you should be using it for the |expectedMuted| comparison. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); Ideally you wouldn't need any timeout and the callback wouldn't be run until the tab has been muted? https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:8: { Open brace should be on same line as function name declaration. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:14: views[0].document.body.appendChild(scriptElem); Instead of this you should be able to do something like "views[0].contentWindow.disableSinewave()" or whatever - if you define the disableSinewave function inside the tab that you're loading, whatever that is. And in that case, I'd compose this with a function "getOnlyTab" then call below with getOnlyTab.disableSinewave() and getOnlyTab.enableSinewave(). https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:20: testTabId_ = tab.id; On 2015/05/26 21:28:47, miu wrote: > kalman@'s more familiar with the JS extension testing than I am. But, IIRC, you > should be calling chrome.test.succeed() for each function passed to > chrome.test.runTests(). Very confusingly, callbackPass (which pass is aliased to) calls succeed() itself: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:63: var checkQuery = function(tabId, queryAttrib, value, expectedFound, callback) { 1. Declare with "function checkQuery" not "var checkQuery = function". 2. Avoid shortened names like "attrib". 3. A long list of arguments like this - particularly when it's mostly booleans - is quite hard to read. Better is to use a single object argument (and document the parameters) such that callers would use: checkQuery({ tabId: 42, attribute: ..., value: ..., expectFound: ..., }, function() { ... }); I don't actually understand the arguments, so that would help me review this as well. Is there a more descriptive name than "checkQuery"? https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:84: function onUpdatedExpect(attrib, expected, otherAttribsDict) { 1. I find the name confusing, perhaps because it doesn't read smoothly; the verb "expect" I think should be at the start of the name, not the end. "expectOnUpdated" almost looks better, but it's not really accurate. Perhaps "assertAttributesOnUpdated"... don't like that much either. 2. See comment above about multiple arguments. "otherAttribsDict" doesn't mean much to me.
Followups for feedback from patch #7. Also, patch #8 has been published to code review. https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc:44: command_line->AppendSwitch(::switches::kEnableTabAudioMuting); On 2015/05/26 at 23:09:46, kalman wrote: > What does this flag actually control? E.g. if the call "chrome::SetTabAudioMuted" fails if it's off, then we should include as part of the API that it fails, rather than silently failing. I'll comment in the place. Addressed elsewhere. https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1200: bool muted = *params->update_properties.muted; On 2015/05/26 at 23:09:46, kalman wrote: > Could you make this (and I may have the names wrong here, and git cl format may choose to change it): > > const char* switch = base::Comment::switches::kEnableTabAudioMuting; > if (base::CommandLine::HasSwitch(switch)) { > chrome::SetTabAudioMuted(contents, *params->update_properties.muted, > extension()->id()); > } else { > WriteToConsole( > WARNING, > base::StringPrintf("Failed to update mute state, --%s must be enabled", > switch)); > } Done (after making a few changes including not naming the variable 'switch' ;)). It looks like the motivation here is to turn off muting if the flag isn't set. Should there also be a flag for turning on/off the API changes? (The creating new APIs website (http://www.chromium.org/developers/design-documents/extensions/proposed-chang...) indicates such but here we are modifying an existing API instead of creating a new one. It seems straightforward to remove the new fields from the tab object and to disable c.t.update (like we do here), but it isn't clear to me how to remove them from c.t.onUpdated and c.t.query.) https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:140: TabEntry(); On 2015/05/26 at 21:28:46, miu wrote: > nit: Can you move this zero-arg constructor to the "private:" section below, then add: > > friend class std::map<int, TabEntry>; > > This would allow the compiler to strictly enforce that the zero-arg constructor is only being used for the std::map<>'s default allocator. ;-) Done. https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:142: // Allow creating a TabEntry via a webcontents. Used when moving On 2015/05/26 at 21:28:46, miu wrote: > This comment isn't quite accurate since the implementation isn't all about tracking tabs moving between windows. Instead, how about: > > // Create a TabEntry associated with, and tracking state changes to, |contents|. Done. This code is the inspiration for including a test for maintaining the audible state when a tab is moved across windows (as I mention in another comment.) https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:101: function tabisUnmutedWhenTabCaptured() { On 2015/05/26 21:28:46, miu wrote: > nit: Capitalize the 'i' in 'is'. done https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:110: if ((typeof changeInfo.mutedCause !== "undefined") && On 2015/05/26 at 23:09:46, kalman wrote: > can also use changeInfo.hasOwnProperty('mutedCause') > > or just forget about it altogether, since you're comparing to |expectedMutedCause| which is a string, so won't be undefined. done https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:111: (changeInfo["mutedCause"] === expectedMutedCause) && On 2015/05/26 21:28:46, miu wrote: > nit: This line and the next should be two more spaces to the right. done https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:112: (changeInfo["muted"] == expectedMuted)) On 2015/05/26 23:09:46, kalman wrote: > Also to the above: not much point using === when comparing to the string > "capture", but arguably you should be using it for the |expectedMuted| > comparison. done https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); On 2015/05/26 23:09:46, kalman wrote: > Ideally you wouldn't need any timeout and the callback wouldn't be run until the > tab has been muted? Right; the test fails without the timeout. In prior discussions, miu suggested that there may be a delay between the time tab capture starts and the notification to the UI to change the tab strip model (and icon) is processed. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2015/05/26 at 23:09:48, kalman wrote: > 2015 Done (also changed muted.js, sinewave.js, and disable_sinewave.js; the code for the latter two came from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...) but I am assuming the copyright should be for the year that the file was created.) https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:8: { On 2015/05/26 at 23:09:48, kalman wrote: > Open brace should be on same line as function name declaration. Done. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:14: views[0].document.body.appendChild(scriptElem); On 2015/05/26 at 23:09:48, kalman wrote: > Instead of this you should be able to do something like "views[0].contentWindow.disableSinewave()" or whatever - if you define the disableSinewave function inside the tab that you're loading, whatever that is. > > And in that case, I'd compose this with a function "getOnlyTab" then call below with getOnlyTab.disableSinewave() and getOnlyTab.enableSinewave(). Would doing this match your suggestion? -- create an .HTML file that gets loaded for this test (instead of the generic one used for other tests) -- file should include sinewave.js and expose enableSinewave and disableSinewave functions -- remove the disable_sinewave.js file https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:20: testTabId_ = tab.id; On 2015/05/26 at 23:09:47, kalman wrote: > On 2015/05/26 21:28:47, miu wrote: > > kalman@'s more familiar with the JS extension testing than I am. But, IIRC, you > > should be calling chrome.test.succeed() for each function passed to > > chrome.test.runTests(). > > Very confusingly, callbackPass (which pass is aliased to) calls succeed() itself: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... Correct. I am using pass here to be consistent with other tabs tests and am using chrome.test.succeed within the tabcapture test to be consistent with those tests. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:32: function audibleUpdateAttempt() { On 2015/05/26 at 21:28:46, miu wrote: > naming nit: How about audibleUpdateAttemptShouldFail()? Done. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:53: function testStaysAudibleAfterChangingWindow() { On 2015/05/26 at 21:28:46, miu wrote: > Not sure if it's necessary to test the state after a tab is moved between windows, but I'm not against keeping this in the tests either. The intention behind this test is because this patch (minimally) touches code in TabsEventRouter::TabInsertedAt and this test gives some assurance that it wasn't broken. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:63: var checkQuery = function(tabId, queryAttrib, value, expectedFound, callback) { On 2015/05/26 at 23:09:48, kalman wrote: > 1. Declare with "function checkQuery" not "var checkQuery = function". > Done > 2. Avoid shortened names like "attrib". Done. (Also, renamed other fields and updated the description to hopefully make it more clear how this works.) > > 3. A long list of arguments like this - particularly when it's mostly booleans - is quite hard to read. Better is to use a single object argument (and document the parameters) such that callers would use: > > checkQuery({ > tabId: 42, > attribute: ..., > value: ..., > expectFound: ..., > }, function() { > ... > }); > > I don't actually understand the arguments, so that would help me review this as well. Is there a more descriptive name than "checkQuery"? Maybe verifyQueryForTab? The purpose of the method is to create an object for a c.t.query and check that the results for a particular tabid match/do not match the expectedValue. I have made some changes but am holding off on changing the method signature until after we discuss this since this involves changing the caller code and I would prefer to not have to do that too many times. One aspect of this function that might be considered confusing is the expectedFound parameter; the reasoning behind it is that to know that a query works correctly I think you need to know both that a match is found and that a nonmatch is not found. An alternative suggestion: remove expectedValue and expectedFound as parameters and return the results of the c.t.query for tabId (undefined if not found) within the callback. The calling code can then do an assert against the expectedValue and handle the found/not found logic. The method can be renamed to something like queryForTab. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:84: function onUpdatedExpect(attrib, expected, otherAttribsDict) { On 2015/05/26 at 23:09:48, kalman wrote: > 1. I find the name confusing, perhaps because it doesn't read smoothly; the verb "expect" I think should be at the start of the name, not the end. "expectOnUpdated" almost looks better, but it's not really accurate. Perhaps "assertAttributesOnUpdated"... don't like that much either. How about expectAfterOnUpdated? > > 2. See comment above about multiple arguments. "otherAttribsDict" doesn't mean much to me. Arguments renamed and description updated to hopefully make this more clear. Like the previous function, I am holding off on making changes that affect the method signature until after we discuss this more.
lgtm % one styling nit: https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:53: function testStaysAudibleAfterChangingWindow() { On 2015/05/27 14:52:02, Jared Sohn wrote: > On 2015/05/26 at 21:28:46, miu wrote: > > Not sure if it's necessary to test the state after a tab is moved between > windows, but I'm not against keeping this in the tests either. > > The intention behind this test is because this patch (minimally) touches code in > TabsEventRouter::TabInsertedAt and this test gives some assurance that it wasn't > broken. Okay. Sounds good to me. :) https://codereview.chromium.org/987583004/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:181: friend class std::map<int, TabEntry>; Thanks for doing this. The style guide requires specific ordering within a section. After "private:" the "friend class" declaration goes first, followed by the zero-arg ctor, then all the data members.
One thing I neglected to mention: I get this message when I try to upload: Your #include order seems to be broken. Send mail to marja@chromium.org if this is not the case. chrome/browser/extensions/api/tabs/tabs_api.cc:21 I have tried to remove the message by putting the #include where it makes sense alphabetically both with and without underscores, but have not had luck yet. I have sent an e-mail and am waiting for a response.
On 2015/05/27 21:06:18, Jared Sohn wrote: > One thing I neglected to mention: I get this message when I try to upload: > > Your #include order seems to be broken. Send mail to > mailto:marja@chromium.org if this is not the case. > chrome/browser/extensions/api/tabs/tabs_api.cc:21 > > I have tried to remove the message by putting the #include where it makes sense > alphabetically both with and without underscores, but have not had luck yet. I > have sent an e-mail and am waiting for a response. The sort order you have in the latest patch set (PS11) should be correct. According to ASCII, the underscore char has value 0x5f and the lowercase 'p' has value 0x70. However, if the sort order is case-insensitive, then perhaps the 'P' (0x50) comes before the underscore? Normally, I do not ignore this presubmit message since I'm usually doing something wrong. However, if you've tried it both ways then, yeah send marja@ an e-mail.
On 2015/05/28 at 18:54:09, miu wrote: > On 2015/05/27 21:06:18, Jared Sohn wrote: > > One thing I neglected to mention: I get this message when I try to upload: > > > > Your #include order seems to be broken. Send mail to > > mailto:marja@chromium.org if this is not the case. > > chrome/browser/extensions/api/tabs/tabs_api.cc:21 > > > > I have tried to remove the message by putting the #include where it makes sense > > alphabetically both with and without underscores, but have not had luck yet. I > > have sent an e-mail and am waiting for a response. > > The sort order you have in the latest patch set (PS11) should be correct. According to ASCII, the underscore char has value 0x5f and the lowercase 'p' has value 0x70. However, if the sort order is case-insensitive, then perhaps the 'P' (0x50) comes before the underscore? > > Normally, I do not ignore this presubmit message since I'm usually doing something wrong. However, if you've tried it both ways then, yeah send marja@ an e-mail. I was doing something wrong. :) The commit I made this morning passes the test.
Still lgtm. https://codereview.chromium.org/987583004/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:181: friend class std::map<int, TabEntry>; On 2015/05/27 20:10:14, miu wrote: > Thanks for doing this. The style guide requires specific ordering within a > section. After "private:" the "friend class" declaration goes first, followed > by the zero-arg ctor, then all the data members. Done.
Responded to the test questions + added a .cc file comment. I'm holding off looking at the new version of the tests until the test conversation is resolved. https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1200: bool muted = *params->update_properties.muted; > It looks like the motivation here is to turn off muting if the flag isn't set. > Should there also be a flag for turning on/off the API changes? (The creating > new APIs website > (http://www.chromium.org/developers/design-documents/extensions/proposed-chang...) > indicates such but here we are modifying an existing API instead of creating a > new one. It seems straightforward to remove the new fields from the tab object > and to disable c.t.update (like we do here), but it isn't clear to me how to > remove them from c.t.onUpdated and c.t.query.) Nah no need for a separate flag. I think the API can just be written once and when tab muting is enabled, the API can start working. An interesting question is actually whether using the API should automatically enable tab muting; in other words, is the flag designed for the ability of tab muting, or is it for the UI? If just the latter, we should give the API free reign. mui? https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); On 2015/05/27 14:52:02, Jared Sohn wrote: > On 2015/05/26 23:09:46, kalman wrote: > > Ideally you wouldn't need any timeout and the callback wouldn't be run until > the > > tab has been muted? > > Right; the test fails without the timeout. In prior discussions, miu suggested > that there may be a delay between the time tab capture starts and the > notification to the UI to change the tab strip model (and icon) is processed. Mm this is problematic then. Timeout is just going to fail somewhere else, like on a bot that's having a bad day, and then the test will be disabled anyway. But yes I'm not surprised that there is a mismatch here - but then, the UI shouldn't be relevant here. It should all be based on the content API events, which should be consistent. Is it definitely not a race in there sometimes, like a bug on our side? An alternative would be to say you have 2 independent events, 1. tab has started capturing, 2. you got the event which you think corresponds to this change, so move the assertions and the chrome.test.succeed() call until after *both* events have arrived. It could look something like: var stream1 = null; var updatedTab = null; function nextStep() { if (!stream1 || !updatedTab) return; chrome.test.assertEq(..); chrome.test.assertEq(..); stream1.stop(); chrome.test.succeed(); } // Listen for tab events, when the relevant one arrives, // set |updatedTab| then call nextStep(). // Start capturing tab, then when it's capturing, // set |stream1| then call nextStep(). Failure will be a timeout, but that's ok, so long as it's a dependable timeout. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2015/05/27 14:52:02, Jared Sohn wrote: > On 2015/05/26 at 23:09:48, kalman wrote: > > 2015 > > Done (also changed muted.js, sinewave.js, and disable_sinewave.js; the code for > the latter two came from > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...) > but I am assuming the copyright should be for the year that the file was > created.) Correct. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:14: views[0].document.body.appendChild(scriptElem); On 2015/05/27 14:52:02, Jared Sohn wrote: > On 2015/05/26 at 23:09:48, kalman wrote: > > Instead of this you should be able to do something like > "views[0].contentWindow.disableSinewave()" or whatever - if you define the > disableSinewave function inside the tab that you're loading, whatever that is. > > > > And in that case, I'd compose this with a function "getOnlyTab" then call > below with getOnlyTab.disableSinewave() and getOnlyTab.enableSinewave(). > > Would doing this match your suggestion? > -- create an .HTML file that gets loaded for this test (instead of the generic > one used for other tests) > -- file should include sinewave.js and expose enableSinewave and disableSinewave > functions > -- remove the disable_sinewave.js file Sounds good to me. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:63: var checkQuery = function(tabId, queryAttrib, value, expectedFound, callback) { > An alternative suggestion: remove expectedValue and expectedFound as parameters > and return the results of the c.t.query for tabId (undefined if not found) > within the callback. The calling code can then do an assert against the > expectedValue and handle the found/not found logic. The method can be renamed > to something like queryForTab. I *think* understand what you mean, and if that understanding is correct, then what you suggest here sounds like the best option. It composes well. https://codereview.chromium.org/987583004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/987583004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1202: if (base::CommandLine::ForCurrentProcess()->HasSwitch(muteSwitch)) { This check should actually be IsTabAudioMutingFeatureEnabled (but it's still useful to mention the flag in the error message; perhaps aliasing muteSwitch isn't that useful though) https://codereview.chromium.org/987583004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1207: "Cannot update mute state for tab being captured")); It's polite to mention the tab ID, also, better details than "cannot update mute state" would be nice, i.e. why it failed. Looks like it's because we're either RECORDING or CAPTURING: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I think it's enough to just say "Cannot update mute state for tab %d, tab has audio or video currently being captured".
https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1200: bool muted = *params->update_properties.muted; On 2015/05/28 23:04:42, kalman wrote: > An interesting question is actually whether using the API should automatically > enable tab muting; in other words, is the flag designed for the ability of tab > muting, or is it for the UI? If just the latter, we should give the API free > reign. mui? It's designed to report whatever state the WebContents is in, whether muting is being controlled from the UI or an extension via this new API. I think the simple answer is, yes, the API should get free reign. We're still discussing (behind-the-scenes) whether muting will become a permanent member of the tab strip UI. ;-) https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); On 2015/05/28 23:04:42, kalman wrote: > On 2015/05/27 14:52:02, Jared Sohn wrote: > But yes I'm not surprised that there is a mismatch here - but then, the UI > shouldn't be relevant here. It should all be based on the content API events, > which should be consistent. Is it definitely not a race in there sometimes, like > a bug on our side? Explanation: The content API events propagate through a "de-duping" heuristic in Browser::NavigationStateChanged() (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) before any changes reach the BrowserTabStripModel and therefore propagate to the tab event dispatcher code. So, the "race" is being artificially introduced by code that attempts to reduce the rate of change in drawing in the browser UI.
https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/987583004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1200: bool muted = *params->update_properties.muted; On 2015/05/28 23:36:59, miu wrote: > On 2015/05/28 23:04:42, kalman wrote: > > An interesting question is actually whether using the API should automatically > > enable tab muting; in other words, is the flag designed for the ability of tab > > muting, or is it for the UI? If just the latter, we should give the API free > > reign. mui? > > It's designed to report whatever state the WebContents is in, whether muting is > being controlled from the UI or an extension via this new API. I think the > simple answer is, yes, the API should get free reign. We're still discussing > (behind-the-scenes) whether muting will become a permanent member of the tab > strip UI. ;-) Awesome. In that case we should actually be able to remove the command line checks from everywhere?
On 2015/05/29 21:28:15, kalman wrote: > Awesome. In that case we should actually be able to remove the command line > checks from everywhere? Let's keep them for now, but not for long. Meaning: The tab strip UI code doesn't understand the difference between "show the mute icon because the tab is muted" and "the audio/mute icon is clickable to toggle mute" (a UI experiment deemed not ready for launch, chrome://flags#enable-tab-audio-muting). The ball's in my court to fix this. BTW--I'm wondering if it makes sense to add an event listener to the chrome.tabs API for when the audio icon in the tab strip is clicked? That would allow extensions to fully replicate the functionality of my UI experiment.
On 2015/05/29 22:37:28, miu wrote: > On 2015/05/29 21:28:15, kalman wrote: > > Awesome. In that case we should actually be able to remove the command line > > checks from everywhere? > > Let's keep them for now, but not for long. Meaning: The tab strip UI code > doesn't understand the difference between "show the mute icon because the tab is > muted" and "the audio/mute icon is clickable to toggle mute" (a UI experiment > deemed not ready for launch, chrome://flags#enable-tab-audio-muting). The > ball's in my court to fix this. > > BTW--I'm wondering if it makes sense to add an event listener to the chrome.tabs > API for when the audio icon in the tab strip is clicked? That would allow > extensions to fully replicate the functionality of my UI experiment. That makes sense.
On 2015/05/29 at 23:02:11, kalman wrote:
> On 2015/05/29 22:37:28, miu wrote:
> > On 2015/05/29 21:28:15, kalman wrote:
> > > Awesome. In that case we should actually be able to remove the command
line
> > > checks from everywhere?
> >
> > Let's keep them for now, but not for long. Meaning: The tab strip UI code
> > doesn't understand the difference between "show the mute icon because the
tab is
> > muted" and "the audio/mute icon is clickable to toggle mute" (a UI
experiment
> > deemed not ready for launch, chrome://flags#enable-tab-audio-muting). The
> > ball's in my court to fix this.
> >
> > BTW--I'm wondering if it makes sense to add an event listener to the
chrome.tabs
> > API for when the audio icon in the tab strip is clicked? That would allow
> > extensions to fully replicate the functionality of my UI experiment.
>
> That makes sense.
Note that this is redundant to:
chrome.tabs.onUpdated.addListener(function(tabId, changeInfo, tab) {
if ((changeInfo.hasOwnProperty("muted")) && (changeInfo["mutedCause"] ==
"user")) {
console.log("indicator clicked!");
}
});
unless you are suggesting that it exist via an extension instead of natively.
I'll probably forget to make this comment after reviewing this CL, so quickly: in your CL description you say "Note: Requires --enable-tab-audio-muting flag to be active (chrome://flags) in order to set a tab's muted state via c.t.update." How much of that is actually true?
On 2015/06/01 at 18:24:29, kalman wrote: > I'll probably forget to make this comment after reviewing this CL, so quickly: in your CL description you say > > "Note: Requires --enable-tab-audio-muting flag to be active (chrome://flags) in order to set a tab's muted state via c.t.update." > > How much of that is actually true? 100%. c.t.update uses the same code path for muting tabs that the indicators do. Both call SetTabAudioMuted in tab_utils.cc, which calls CanToggleAudioMute which calls IsTabAudioMutingFeatureEnabled which checks for that flag.
Just test comments now. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); On 2015/05/28 23:36:59, miu wrote: > On 2015/05/28 23:04:42, kalman wrote: > > On 2015/05/27 14:52:02, Jared Sohn wrote: > > But yes I'm not surprised that there is a mismatch here - but then, the UI > > shouldn't be relevant here. It should all be based on the content API events, > > which should be consistent. Is it definitely not a race in there sometimes, > like > > a bug on our side? > > Explanation: The content API events propagate through a "de-duping" heuristic in > Browser::NavigationStateChanged() > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) > before any changes reach the BrowserTabStripModel and therefore propagate to the > tab event dispatcher code. So, the "race" is being artificially introduced by > code that attempts to reduce the rate of change in drawing in the browser UI. Ok, thanks for the explanation. Would my suggestion fix the potential flakiness? Jared, could you give it a go? https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2015/05/28 23:04:42, kalman wrote: > On 2015/05/27 14:52:02, Jared Sohn wrote: > > On 2015/05/26 at 23:09:48, kalman wrote: > > > 2015 > > > > Done (also changed muted.js, sinewave.js, and disable_sinewave.js; the code > for > > the latter two came from > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...) > > but I am assuming the copyright should be for the year that the file was > > created.) > > Correct. btw we also now leave the (c) out, for the record. https://codereview.chromium.org/987583004/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.cc:426: const char* muteSwitch = switches::kEnableTabAudioMuting; I don't think this alias is necessary, though for the record (and if you really need it) then it should be mute_switch not muteSwitch. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/sinewave.js (right): https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/sinewave.js:7: var curTestIdx = 0; am I missing something, or is curTestIdx never actually changed? (btw should be currentTextIdx or currentTextIndex or even testIndex or whatever - point being, minimise abbreviations, unless it's a super common one, often used to avoid a collision with a keyword or whatever - I'm guilty of using "obj"). https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/sinewave.js:9: this.audioContext = new AudioContext(); I'd rather not use |this| here. Trivia: it actually used to be the case that using |this| here wouldn't work, because v8 would set it to the *calling* window, not the *receiver* window. So, doing getOnlyTab().enableSineWave(); would call enableSinceWave on the current window (i.e. the background page), not the tab. Anyway, that's fixed, but looking at this file in isolation is pretty hard to see what's going on. The variables you're setting on it also seem dangerously prone to being overridden. If this were production code or something you could do something more fancy than this, but I'd approximate modularity like: An approximation of JS modularity would look like: window.sinewave = { enable: function(win) { var data = win.sinewaveData_; if (!data) { data = {}; win.sinewaveData_ = data; } if (!data.audioContext) ... etc ...; if (!data.oscillator...) ... etc ...; }, disable: function(obj) { obj.sinewave_.oscillator.stop(); } }; then the places where you call getOnlyTab().enableSineWave() you'd instead call window.sinewave.enable(getOnlyTab()), and you only need to include the sinewave.js file on the background page. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right): https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:60: // Do a c.t.query and return the found tab (or undefined) in a callback Huh, funny, I guess we should support a tabId property in query. A little bit silly needing to write code like this - but I guess it's for a test, so makes sense. Btw I'd find the comment "Like chrome.tabs.query, but with the ability to filter by |tabId| as well. Returns the found tab, or null" more useful. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:68: callback(foundTabs.length ? foundTabs[0] : undefined); epic nit but this isn't "undefined", because we know what it is - it's nothing (in fact, no object). Therefore it should be "null". Kind of like how document.getElementById() returns "null" if it's not found. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:93: if (queryableAttrib in changeInfo) { use hasOwnProperty, otherwise it will include properties on Object (i.e. look up prototype chain).
I uploaded a new patch. Comments below. https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); > Ok, thanks for the explanation. Would my suggestion fix the potential flakiness? > Jared, could you give it a go? miu: Are you saying that there is a delay before the tab dispatcher code knows a tab is muted, especially prior to tab capture? If so that makes sense based on the behavior I am witnessing. This is a little different than the potential problem described in my earlier comment (and that kalman's suggestion is based on.) kalman: I don't think your solution will work because we care about the order between making the tab muted and having tab capture unmute it. If we allow them both to run independently, then sometimes it will mute and then unmute the tab like we want and sometimes it will try to mute a tab after it is captured (which will result in an error message.) After quite a bit of playing with the code, here is the behavior that I am observing: * The basic problem is that we are not getting any onUpdated events if we remove the setTimeout. * If I change update code to set {pinned: true}, it does run the onUpdated event. * If I add chrome.tabs.getCurrent(function(tab) { console.log(tab.muted); console.log(tab.mutedCause); }); after the tab capture, I find that the mutedCause is always "capture". (I had thought the problem might be a timing issue with when the state was set to captured, but this seems not to be the case.) * In the above test, muted is always false if the timeout is sufficiently long (170 ms on my machine) and otherwise varies between true and false. * I did some ad hoc testing of combining chrome.tabs.update({"muted"}) with getCurrent and onUpdated (basically no tab capture), and that worked okay. So I think the bug can be described as: If we mute a tab and then tab capture it without waiting long enough before doing so, then the onUpdated events do not trigger and a race condition can cause muted to be assigned the wrong value. If there is a maximum delay before updating the tab event dispatcher code, perhaps we can just add a setTimeout a little longer than that and be assured that it will work consistently? https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/audible.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/audible.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2015/06/01 20:33:56, kalman wrote: > On 2015/05/28 23:04:42, kalman wrote: > > On 2015/05/27 14:52:02, Jared Sohn wrote: > > > On 2015/05/26 at 23:09:48, kalman wrote: > > > > 2015 > > > > > > Done (also changed muted.js, sinewave.js, and disable_sinewave.js; the code > > for > > > the latter two came from > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...) > > > but I am assuming the copyright should be for the year that the file was > > > created.) > > > > Correct. > > btw we also now leave the (c) out, for the record. Updated in new files. https://codereview.chromium.org/987583004/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right): https://codereview.chromium.org/987583004/diff/220001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.cc:426: const char* muteSwitch = switches::kEnableTabAudioMuting; Oversight; removed. On 2015/06/01 at 20:33:56, kalman wrote: > I don't think this alias is necessary, though for the record (and if you really need it) then it should be mute_switch not muteSwitch. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/sinewave.js (right): https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/sinewave.js:7: var curTestIdx = 0; On 2015/06/01 at 20:33:57, kalman wrote: > am I missing something, or is curTestIdx never actually changed? Just a note on this: I didn't write most of this code (it came from /src/chrome/test/data/extensions/api_test/cast_streaming/end_to_end_sender.js&l=54) but I should have cleaned it up more (especially in terms of what portions I copied over). Code rewritten. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/sinewave.js:9: this.audioContext = new AudioContext(); Done. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right): https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:60: // Do a c.t.query and return the found tab (or undefined) in a callback Yeah, I like that better; updated. On 2015/06/01 at 20:33:57, kalman wrote: > Huh, funny, I guess we should support a tabId property in query. A little bit silly needing to write code like this - but I guess it's for a test, so makes sense. > > Btw I'd find the comment "Like chrome.tabs.query, but with the ability to filter by |tabId| as well. Returns the found tab, or null" more useful. https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:68: callback(foundTabs.length ? foundTabs[0] : undefined); On 2015/06/01 at 20:33:57, kalman wrote: > epic nit but this isn't "undefined", because we know what it is - it's nothing (in fact, no object). Therefore it should be "null". Kind of like how document.getElementById() returns "null" if it's not found. Done. (A previous iteration of the code would pass the query value into the callback and would set it to undefined if the tab didn't exist.) https://codereview.chromium.org/987583004/diff/220001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:93: if (queryableAttrib in changeInfo) { On 2015/06/01 at 20:33:57, kalman wrote: > use hasOwnProperty, otherwise it will include properties on Object (i.e. look up prototype chain). Done.
https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:126: }, 200); On 2015/06/02 08:54:38, Jared Sohn wrote: > > Ok, thanks for the explanation. Would my suggestion fix the potential > flakiness? > > Jared, could you give it a go? > > miu: Are you saying that there is a delay before the tab dispatcher code knows a > tab is muted, especially prior to tab capture? If so that makes sense based on > the behavior I am witnessing. This is a little different than the potential > problem described in my earlier comment (and that kalman's suggestion is based > on.) > > kalman: I don't think your solution will work because we care about the order > between making the tab muted and having tab capture unmute it. If we allow them > both to run independently, then sometimes it will mute and then unmute the tab > like we want and sometimes it will try to mute a tab after it is captured (which > will result in an error message.) > > After quite a bit of playing with the code, here is the behavior that I am > observing: > > * The basic problem is that we are not getting any onUpdated events if we remove > the setTimeout. > > * If I change update code to set {pinned: true}, it does run the onUpdated > event. > > * If I add > > chrome.tabs.getCurrent(function(tab) { > console.log(tab.muted); > console.log(tab.mutedCause); > }); > > after the tab capture, I find that the mutedCause is always "capture". (I had > thought the problem might be a timing issue with when the state was set to > captured, but this seems not to be the case.) > > * In the above test, muted is always false if the timeout is sufficiently long > (170 ms on my machine) and otherwise varies between true and false. > > * I did some ad hoc testing of combining chrome.tabs.update({"muted"}) with > getCurrent and onUpdated (basically no tab capture), and that worked okay. > > > So I think the bug can be described as: > > If we mute a tab and then tab capture it without waiting long enough before > doing so, then the onUpdated events do not trigger and a race condition can > cause muted to be assigned the wrong value. > > If there is a maximum delay before updating the tab event dispatcher code, > perhaps we can just add a setTimeout a little longer than that and be assured > that it will work consistently? Ok: should we just forget about this portion of the test then? If there's no way to guarantee that if you mute a tab, then turn around right away and query its mute state that it's updated... then testing that non-guaranteeable behavior seems pointless. Or another option would be in the test setup to remove the the timeout that miu@ refers to... but I'm not sure I trust that, either. Anyway, I think that a test of 1. query mute state, check is false 2. mute tab 3. wait for update 4. assert that update has mute is true 5. query mute state, check is true is perfectly strong.
>If there's no way to guarantee that if you mute a tab, then turn around right away and query its mute state that it's updated This actually works reliably and we do it in muted.js. The problem seems to be only if you update muted before starting a tab capture. If you follow miu's link into ScheduleUIUpdate (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...), you'll see that it updates the UI every kUIUpdateCoalescingTimeMS = 200 ms (which is pretty close to the 170 ms that I have found experimentally.) Normally, when you make a request in JavaScript you get the response back asynchronously; presumably Chrome waits for the coalescing to finish before returning a response. tabCapture.capture gets the muted status internally so I am guessing that it skips waiting for the coalescing. If this is so, then tabcapture.capture should be updated to include this wait. (Or we can hack around this by doing a 200 ms wait within the test.)
An update on this: I changed the constant from 200 ms to 2000 ms in Chromium and recompiled which caused the test to fail. When I changed the test to do a setTimeout for 2000, it worked again. Thus, this seems to be the reason why we need the setTimeout. I am less sure about the explanation I attempted in my previous comment (there is still a callback prior to starting the tabcapture and I think tabcapture gets information from webcontents which I think doesn't wait for the UI to coalesce to be updated), but I think the test as it was (with perhaps a slightly longer timeout to prevent race conditions and a comment that indicates the reason for the setTimeout) should be acceptable.
On 2015/06/03 01:51:35, Jared Sohn wrote:
> An update on this:
>
> I changed the constant from 200 ms to 2000 ms in Chromium and recompiled which
> caused the test to fail. When I changed the test to do a setTimeout for 2000,
> it worked again. Thus, this seems to be the reason why we need the
setTimeout.
>
> I am less sure about the explanation I attempted in my previous comment (there
> is still a callback prior to starting the tabcapture and I think tabcapture
gets
> information from webcontents which I think doesn't wait for the UI to coalesce
> to be updated), but I think the test as it was (with perhaps a slightly longer
> timeout to prevent race conditions and a comment that indicates the reason for
> the setTimeout) should be acceptable.
Like I said, picking a single point in time to run an assertion is never
acceptable because you can't predict the environment in which tests are run,
especially when automated. Automated tests run in VMs in a bunch of different
configurations, some slow, some fast, probably an unreliable system clock - best
case it's going to annoy people occasionally when it fails, worst case it's
going to get disabled for failing too frequently and *really* annoy people.
Other things to consider: if somebody increases kUIUpdateCoalescingTimeMS this
test will start failing. If someone decreases it, the test will continue to take
longer than necessary. You *could* poll until the condition is set using
setTimeout(<small value>) - if the test starts to fail it will start timing out,
but importantly, it will time out reliably and it would be immediately clear
which patch caused the failure.
So - that's my order of preference. 1. Do something to make the timeout entirely
unnecessary. 2. Write a little framework for polling a condition and backing
off.
2. Could be fun really. I'd look at writing a general chrome.test utility for
it, like,
// Runs the function until stop() is called. Each poll iteration
// is triggered by calling next().
chrome.test.poll(function(next, stop) {
// For example:
chrome.tabs.query(tabId, function(tab) {
if (tab.muted)
stop();
else
next();
});
});
chrome.test.poll can implement backoff, use chrome.test.addCallback, etc.
Oops, I should have thought more about what "bot having a bad day" meant. (I
haven't worked a lot with centralized automated testing.)
I have some new insight into what may be the reason for the timing test: perhaps
the chrome.tabs.onUpdated event is not being run because we are rapidly muting
and then unmuting (via tabcapture) the tab before Chromium generates the
onUpdated event. The only phenomena I am not seeing this explain is why the tab
object sometimes is muted and sometimes is unmuted after we tab capture in this
manner.
I have written some code using polling that seems like it will work, but I
needed to make a couple of changes compared to your suggestion. First, I added
a setup callback since we want to vary the timeout in the middle of the sequence
of {mute a tab, wait a bit, start tab capture (which unmutes it) }. Second, to
poll this multiple times, we need to close the stream but for it to succeed the
stream cannot be closed too soon. Thus, I am passing along the initial timeout
as a parameter using the reasoning that eventually the timeout will be long
enough for both places (doesn't seem ideal, but I'm not sure if there is a
better alternative.)
What are your thoughts on this approach? Would it still be considered general
enough to make it chrome.test.poll or should I just write it as a method within
this test?
On 2015/06/03 22:43:30, Jared Sohn wrote:
> Oops, I should have thought more about what "bot having a bad day" meant. (I
> haven't worked a lot with centralized automated testing.)
>
> I have some new insight into what may be the reason for the timing test:
perhaps
> the chrome.tabs.onUpdated event is not being run because we are rapidly muting
> and then unmuting (via tabcapture) the tab before Chromium generates the
> onUpdated event. The only phenomena I am not seeing this explain is why the
tab
> object sometimes is muted and sometimes is unmuted after we tab capture in
this
> manner.
>
> I have written some code using polling that seems like it will work, but I
> needed to make a couple of changes compared to your suggestion. First, I
added
> a setup callback since we want to vary the timeout in the middle of the
sequence
> of {mute a tab, wait a bit, start tab capture (which unmutes it) }.
In that case wouldn't you only need to poll over {wait a bit}, and/or call poll
more than once? Like, you can do
muteATab();
chrome.test.poll(function(next, stop) {
if (muted) stop();
else next();
});
startTabCapture();
chrome.test.poll(function(next, stop) {
if (!muted) stop();
else next();
});
> Second, to
> poll this multiple times, we need to close the stream but for it to succeed
the
> stream cannot be closed too soon. Thus, I am passing along the initial
timeout
> as a parameter using the reasoning that eventually the timeout will be long
> enough for both places (doesn't seem ideal, but I'm not sure if there is a
> better alternative.)
I don't understand this point sorry, how does passing along an initial timeout
help?
>
> What are your thoughts on this approach? Would it still be considered general
> enough to make it chrome.test.poll or should I just write it as a method
within
> this test?
> In that case wouldn't you only need to poll over {wait a bit}, and/or call
poll more than once? Like, you can do
>
> muteATab();
> chrome.test.poll(function(next, stop) {
> if (muted) stop();
> else next();
> });
> startTabCapture();
> chrome.test.poll(function(next, stop) {
> if (!muted) stop();
> else next();
> });
The code you suggest here should work for verifying that the tab object's muted
state gets updated but I have been focusing more on getting the onUpdated code
to run. For that to happen, we need to repeat the full sequence of muting a
tab, waiting a bit, and then starting a tab capture, where the "wait a bit"
interval increases exponentially.
> I don't understand this point sorry, how does passing along an initial timeout
help?
The actual sequence is {muting a tab, waiting a bit, starting a tab capture,
waiting another bit, closing the stream}. We need to know what 'waiting another
bit" means and we cannot poll that part of the sequence separately, so I am
reusing the original "wait a bit" for "wait another bit". The two times likely
aren't supposed to be equal, but so long as they both increase and it doesn't
approach the test timeout, the test should eventually pass.
https://codereview.chromium.org/987583004/diff/240001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:104: chrome.test.listenForever(chrome.tabs.onUpdated, you should also stop listening to this, var stopListening = chrome.test.listenForever...; then call stopListening before calling chrome.test.succeed(). in fact that may automatically call succeed :\ https://codereview.chromium.org/987583004/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:107: var expectedMuted = false; I actually find these harder to read than just if (changeInfo.muted === false && changeInfo.mutedCase == 'capture') { chrome.test.assertEq(false, updatedTab.muted); chrome.test.assertEq('capture', updatedTab.mutedCause); ... } https://codereview.chromium.org/987583004/diff/240001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:124: }); Ok, I see your point. This is hard to fit into some sort of framework. Maybe after the chrome.tabs.update() you should be watching for a muted change, and then start the capture now that you know it's muted, and then wait for the capture change? would that work? sorry if you've already gone through all of this.
>then call stopListening before calling chrome.test.succeed(). in fact that may automatically call succeed :\ It did. :) >I actually find these harder to read than just Agreed. >Ok, I see your point. This is hard to fit into some sort of framework. Maybe after the chrome.tabs.update() you should be watching for a muted change, and then start the capture now that you know it's muted, and then wait for the capture change? would that work? I actually started out that way but I remember running into some difficulty. But after trying it again, it works, so I will change the code to work that way.
On 2015/06/04 00:44:27, Jared Sohn wrote: > >then call stopListening before calling chrome.test.succeed(). in fact that may > automatically call succeed :\ > > It did. :) > > >I actually find these harder to read than just > > Agreed. > > >Ok, I see your point. This is hard to fit into some sort of framework. Maybe > after the chrome.tabs.update() you should be watching for a muted change, and > then start the capture now that you know it's muted, and then wait for the > capture change? would that work? > > I actually started out that way but I remember running into some difficulty. > But after trying it again, it works, so I will change the code to work that way. so I should review this again?
Yes, please. (I looked over my notes for when I changed approaches and as far as I can tell, I switched to this due to some silly bug that I didn't make this time.) On 2015/06/05 at 19:32:08, kalman wrote: > On 2015/06/04 00:44:27, Jared Sohn wrote: > > >then call stopListening before calling chrome.test.succeed(). in fact that may > > automatically call succeed :\ > > > > It did. :) > > > > >I actually find these harder to read than just > > > > Agreed. > > > > >Ok, I see your point. This is hard to fit into some sort of framework. Maybe > > after the chrome.tabs.update() you should be watching for a muted change, and > > then start the capture now that you know it's muted, and then wait for the > > capture change? would that work? > > > > I actually started out that way but I remember running into some difficulty. > > But after trying it again, it works, so I will change the code to work that way. > > so I should review this again?
lgtm, let's go! https://codereview.chromium.org/987583004/diff/260001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tab_capture/api_tests.js (right): https://codereview.chromium.org/987583004/diff/260001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tab_capture/api_tests.js:106: if ((changeInfo["muted"] === true)) { outer parens not necessary
The CQ bit was checked by jared.sohn@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/987583004/#ps260001 (title: "Change tabIsUnmutedWhenTabCaptured test to not require a timeout")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jared.sohn@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/987583004/#ps280001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jared.sohn@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/987583004/#ps300001 (title: "Make TabsEventRouter a friend to TabEntry so that it will build on some platforms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The error that we are getting now on some platforms is related to moving the zero-arg constructor for TabEntry into the private section. I tried to make TabEventRouter a friend to TabEntry in case that would help, but it did not. Any ideas? (I am guessing that making the constructor public again would work, but that wouldn't restrict access.) I also made a few changes to rebase for more recent code. There were four places where the patch failed: two were because other header files had been added, another was because a method signature had changed, and the last was because there was a change to checking if an object which should get replaced by our code anyway. (See tabs_event_router.cc (3 changes) and tabs_api.cc (1 change).)
https://codereview.chromium.org/987583004/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_event_router.h (right): https://codereview.chromium.org/987583004/diff/300001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_event_router.h:201: std::map<int, TabEntry> tab_entries_; I would do this: using TabEntryMap = std::map<int, linked_ptr<TabEntry>>; TabEntryMap tab_entries_; then you don't need any zero-arg constructor at all, no friends, plus you avoid copying the struct.
The CQ bit was checked by jared.sohn@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/987583004/#ps320001 (title: "TabEntry map stores entries as linked_ptrs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jared.sohn@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jared.sohn@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by jared.sohn@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by jared.sohn@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/987583004/#ps340001 (title: "Including missing html/js files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by jared.sohn@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/987583004/#ps360001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987583004/360001
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/09a3ccc44eb5b7552d6ec49dc8fb46678dd4025f Cr-Commit-Position: refs/heads/master@{#337981} |
