|
|
Created:
4 years, 8 months ago by Taylor.Hoon Modified:
4 years, 8 months ago CC:
chromium-reviews, blink-reviews, tommyw+watchlist_chromium.org, mcasas+watch+mediastream_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis is the first step towards the deprecation and removal of this attribute.
BUG=598704
Committed: https://crrev.com/c13ddaa06b8eb33072db24d79f9d338c8ac0213d
Cr-Commit-Position: refs/heads/master@{#388171}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove |remote| and |readonly| members of MediaStreamTrack #
Total comments: 3
Patch Set 3 : Add a counter for the |remote| member of MediaStreamTrack #
Messages
Total messages: 35 (15 generated)
Description was changed from ========== Remove |remote| and |readonly| members of MediaStreamTrack BUG= ========== to ========== Remove |remote| and |readonly| members of MediaStreamTrack. First step is to add UMA counters for each members to check how much they're used in real web sites. BUG=598704 ==========
taylor.hoon@gmail.com changed reviewers: + hta@chromium.org
Now I begin the process for removing |remote| and |readonly| members of MediaStreamTrack. For that, first step is to add UMA counters for each member to check how much they're used in real web sites.
Looks good, but please complete the CL: run update_use_counter_feature_enum.py in chromium/src/tools/metrics/histograms/ to update the UMA mapping, and add the resulting XML file to the CL.
guidou@chromium.org changed reviewers: + guidou@chromium.org
Note also that the description of the CL does not match its content since you are not removing anything yet. https://codereview.chromium.org/1888023002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right): https://codereview.chromium.org/1888023002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:35: [MeasureAs=MediaStreamTrackReadonly] readonly attribute boolean _readonly; You are adding a new |_readonly| attribute instead of adding a counter to an existing attribute.
Description was changed from ========== Remove |remote| and |readonly| members of MediaStreamTrack. First step is to add UMA counters for each members to check how much they're used in real web sites. BUG=598704 ========== to ========== First step is to add UMA counters for each members to check how much they're used in real web sites. BUG=598704 ==========
On 2016/04/14 13:30:21, hta - Chromium wrote: > Looks good, but please complete the CL: > > run update_use_counter_feature_enum.py in chromium/src/tools/metrics/histograms/ > to update the UMA mapping, and add the resulting XML file to the CL. I update resulting XML file to the CL.
lgtm I think you have to get some more approvals, though.
https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right): https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:35: [MeasureAs=MediaStreamTrackReadonly] readonly attribute boolean _readonly; I don't think you should be adding this attribute, including its counter.
On 2016/04/15 08:19:17, Guido Urdaneta wrote: > https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right): > > https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:35: > [MeasureAs=MediaStreamTrackReadonly] readonly attribute boolean _readonly; > I don't think you should be adding this attribute, including its counter. @Gauido Urdaneta 1. Though I tried to edit the description of Patch Set2, but it's not edited in the page. So I will write a right description with new Patch Set. 2. I couldn't find how other users access "readonly" attribute. Is it possible to use it without IDL?? Then is it enough to call UseCounter::counter() function with "MediaStreamTrackReadonly" in MediaStreamTrack::readonly()??
Description was changed from ========== First step is to add UMA counters for each members to check how much they're used in real web sites. BUG=598704 ========== to ========== This is the first step towards the deprecation and removal of these attributes. BUG=598704 ==========
On 2016/04/15 08:37:03, Taylor.Hoon wrote: > On 2016/04/15 08:19:17, Guido Urdaneta wrote: > > > https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl > (right): > > > > > https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:35: > > [MeasureAs=MediaStreamTrackReadonly] readonly attribute boolean _readonly; > > I don't think you should be adding this attribute, including its counter. > > @Gauido Urdaneta > 1. Though I tried to edit the description of Patch Set2, but it's not edited in > the page. > So I will write a right description with new Patch Set. > I edited the CL description. > 2. I couldn't find how other users access "readonly" attribute. > Is it possible to use it without IDL?? > Then is it enough to call UseCounter::counter() function with > "MediaStreamTrackReadonly" in MediaStreamTrack::readonly()?? Yes, you should call UseCounter::count() inside MediaStreamTrack::readonly() if you want to count those calls.
Notes.... https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1126: MediaStreamTrackRemote = 1306, Note: People add new numbers in this list *all the time*, so it's wise to rebase your CL just before you submit it - you can't check in a CL that uses a number that's already used. https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl (right): https://codereview.chromium.org/1888023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl:35: [MeasureAs=MediaStreamTrackReadonly] readonly attribute boolean _readonly; On 2016/04/15 08:19:17, Guido Urdaneta wrote: > I don't think you should be adding this attribute, including its counter. Oops - I missed that. Its absence means that we never implemented it, which means that we don't have to worry about removing it. Just remove it (and the corresponding counter) from the CL.
lgtm
The CQ bit was checked by hta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888023002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888023002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
core/ LGTM You need to get an LG for histograms.xml though.
Description was changed from ========== This is the first step towards the deprecation and removal of these attributes. BUG=598704 ========== to ========== This is the first step towards the deprecation and removal of this attribute. BUG=598704 ==========
hta@chromium.org changed reviewers: + isherman@chromium.org
isherman: please review histograms.xml
histograms.xml lgtm
The CQ bit was checked by guidou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888023002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888023002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888023002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888023002/40001
Message was sent while issue was closed.
Description was changed from ========== This is the first step towards the deprecation and removal of this attribute. BUG=598704 ========== to ========== This is the first step towards the deprecation and removal of this attribute. BUG=598704 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== This is the first step towards the deprecation and removal of this attribute. BUG=598704 ========== to ========== This is the first step towards the deprecation and removal of this attribute. BUG=598704 Committed: https://crrev.com/c13ddaa06b8eb33072db24d79f9d338c8ac0213d Cr-Commit-Position: refs/heads/master@{#388171} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c13ddaa06b8eb33072db24d79f9d338c8ac0213d Cr-Commit-Position: refs/heads/master@{#388171}
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |