Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19722011:
Added a placeholder new media-internals page under a flag. (Closed)
|
Created:
7 years, 5 months ago by Ty Overby Modified:
7 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded a placeholder new media-internals page under a flag.
A blank hello-world html file under content/browser/resources/media/new
that replaces chrome://media-internals when ran with the flag
--enable-new-media-internals.
BUG=260005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212760
Patch Set 1 #
Total comments: 32
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/19722011/diff/1/content/browser/media/media_i... File content/browser/media/media_internals_ui.cc (right): https://codereview.chromium.org/19722011/diff/1/content/browser/media/media_i... content/browser/media/media_internals_ui.cc:27: source->SetDefaultResource(IDR_MEDIA_INTERNALS_NEW_HTML); nit: return early here instead of sticking the other code in an else branch https://codereview.chromium.org/19722011/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/19722011/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_process_host_impl.cc:914: switches::kEnableNewMediaInternals, we only propagate switches to the render process if the render process needs them AFAICT since the code needing the switch is in the browser process (i.e., content/browser/media/media_internals_ui.cc), you shouldn't need this line can you try removing and seeing if everything still works? https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... File content/browser/resources/media/new/media_internals.css (right): https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.css:2: background: red; let's make this CL even simpler and ditch the CSS for now https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... File content/browser/resources/media/new/media_internals.html (right): https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:1: <html> copyright https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:1: <html> <!DOCTYPE html> at the very top this specifies that this is indeed an HTML5 page and enables strict mode http://www.w3.org/html/wg/drafts/html/master/syntax.html#the-doctype https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:3: <link rel="stylesheet" href="media_internals.css"> you're missing a few other things please refer to the HTML section of the style guide http://dev.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:5: <title> Media-Internals </title> nit: remove spaces on the side I'd also just title this "Media Internals" without the - https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... File content/browser/resources/media/new/media_internals.js (right): https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.js:1: var media = {} copyright https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.js:8: media.initialize = thunk; OOC do we need these to make things work w/ the empty page? https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd File content/content_resources.grd (right): https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd... content/content_resources.grd:22: remove extra blank line https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd... content/content_resources.grd:25: remove extra blank line https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd... content/content_resources.grd:28: remove extra blank lines https://codereview.chromium.org/19722011/diff/1/content/public/common/content... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.cc:410: // Enables the new media-internals page. nit: chrome://media-internals https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.cc:410: // Enables the new media-internals page. can you link to a bug for the new media-internals page so folks can check out the progress? if we don't have a bug filed for the new page, can you file one? https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.cc:411: const char kEnableNewMediaInternals[] = "enable-new-media-internals"; line up that = sign https://codereview.chromium.org/19722011/diff/1/content/public/common/content... File content/public/common/content_switches.h (right): https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.h:141: CONTENT_EXPORT extern const char kEnableNewMediaInternals[]; I believe you don't need CONTENT_EXPORT on this switch as it it's only accessed within the content module
https://codereview.chromium.org/19722011/diff/1/content/browser/media/media_i... File content/browser/media/media_internals_ui.cc (right): https://codereview.chromium.org/19722011/diff/1/content/browser/media/media_i... content/browser/media/media_internals_ui.cc:27: source->SetDefaultResource(IDR_MEDIA_INTERNALS_NEW_HTML); On 2013/07/19 01:58:49, scherkus wrote: > nit: return early here instead of sticking the other code in an else branch Done. https://codereview.chromium.org/19722011/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/19722011/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_process_host_impl.cc:914: switches::kEnableNewMediaInternals, On 2013/07/19 01:58:49, scherkus wrote: > we only propagate switches to the render process if the render process needs > them > > AFAICT since the code needing the switch is in the browser process (i.e., > content/browser/media/media_internals_ui.cc), you shouldn't need this line > > can you try removing and seeing if everything still works? Done. You were right. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... File content/browser/resources/media/new/media_internals.css (right): https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.css:2: background: red; On 2013/07/19 01:58:49, scherkus wrote: > let's make this CL even simpler and ditch the CSS for now I added it to see if I needed extra gyp rules, but apparently I don't, so I'll remove it. Done. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... File content/browser/resources/media/new/media_internals.html (right): https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:1: <html> On 2013/07/19 01:58:49, scherkus wrote: > copyright Done. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:1: <html> On 2013/07/19 01:58:49, scherkus wrote: > <!DOCTYPE html> at the very top > > this specifies that this is indeed an HTML5 page and enables strict mode > http://www.w3.org/html/wg/drafts/html/master/syntax.html#the-doctype Done. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:3: <link rel="stylesheet" href="media_internals.css"> On 2013/07/19 01:58:49, scherkus wrote: > you're missing a few other things > > please refer to the HTML section of the style guide > http://dev.chromium.org/developers/web-development-style-guide Done. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.html:5: <title> Media-Internals </title> On 2013/07/19 01:58:49, scherkus wrote: > nit: remove spaces on the side > > I'd also just title this "Media Internals" without the - Done. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... File content/browser/resources/media/new/media_internals.js (right): https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.js:1: var media = {} On 2013/07/19 01:58:49, scherkus wrote: > copyright Done. https://codereview.chromium.org/19722011/diff/1/content/browser/resources/med... content/browser/resources/media/new/media_internals.js:8: media.initialize = thunk; On 2013/07/19 01:58:49, scherkus wrote: > OOC do we need these to make things work w/ the empty page? It keeps 'method media.onMediaEvent' from being spammed to the console non stop. The messagesReceived counter was there to make sure that we were actually getting messages. https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd File content/content_resources.grd (right): https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd... content/content_resources.grd:22: On 2013/07/19 01:58:49, scherkus wrote: > remove extra blank line Done. https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd... content/content_resources.grd:25: On 2013/07/19 01:58:49, scherkus wrote: > remove extra blank line Done. https://codereview.chromium.org/19722011/diff/1/content/content_resources.grd... content/content_resources.grd:28: On 2013/07/19 01:58:49, scherkus wrote: > remove extra blank lines Done. https://codereview.chromium.org/19722011/diff/1/content/public/common/content... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.cc:410: // Enables the new media-internals page. On 2013/07/19 01:58:49, scherkus wrote: > nit: chrome://media-internals Done. https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.cc:410: // Enables the new media-internals page. On 2013/07/19 01:58:49, scherkus wrote: > can you link to a bug for the new media-internals page so folks can check out > the progress? > > if we don't have a bug filed for the new page, can you file one? Done. https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.cc:411: const char kEnableNewMediaInternals[] = "enable-new-media-internals"; On 2013/07/19 01:58:49, scherkus wrote: > line up that = sign Done. https://codereview.chromium.org/19722011/diff/1/content/public/common/content... File content/public/common/content_switches.h (right): https://codereview.chromium.org/19722011/diff/1/content/public/common/content... content/public/common/content_switches.h:141: CONTENT_EXPORT extern const char kEnableNewMediaInternals[]; On 2013/07/19 01:58:49, scherkus wrote: > I believe you don't need CONTENT_EXPORT on this switch as it it's only accessed > within the content module You were right. Done.
few more tiny nits https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... File content/browser/resources/media/new/media_internals.html (right): https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.html:4: Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: it's 2013! https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... File content/browser/resources/media/new/media_internals.js (right): https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: it's 2013! https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.js:2: // // Use of this source code is governed by a BSD-style license that can be remove comment craziness here https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.js:7: var messagesReceived = 0; if we're not using this just remove it imo https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.js:8: var thunk = function (){ nit: instead of "thunk" let's be a bit more descriptive function doNothing() {}; https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.js:15: media.onReceiveEverything = thunk; nit: don't bother w/ the alignment https://codereview.chromium.org/19722011/diff/9001/content/public/common/cont... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/19722011/diff/9001/content/public/common/cont... content/public/common/content_switches.cc:411: // https://codereview.chromium.org/18889006/ that's a CL not a crbug ;) can you file a bug + link to it here + update your CL description to change BUG=none to BUG=YOUR_BUG_THAT_YOU'RE_GOING_TO_FILE
https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... File content/browser/resources/media/new/media_internals.html (right): https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.html:4: Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/19 18:26:13, scherkus wrote: > nit: it's 2013! Haha, that's what I get for copy/pasta. Done. https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... File content/browser/resources/media/new/media_internals.js (right): https://codereview.chromium.org/19722011/diff/9001/content/browser/resources/... content/browser/resources/media/new/media_internals.js:2: // // Use of this source code is governed by a BSD-style license that can be On 2013/07/19 18:26:13, scherkus wrote: > remove comment craziness here Done.
LGTM I believe you'll need some OWNERS reviews as well
On 2013/07/19 19:21:28, scherkus wrote: > LGTM > > I believe you'll need some OWNERS reviews as well How do I get those again? I already have Avi@ and tommi@ listed as reviewers.
On 2013/07/19 19:25:42, Ty Overby wrote: > On 2013/07/19 19:21:28, scherkus wrote: > > LGTM > > > > I believe you'll need some OWNERS reviews as well > > How do I get those again? I already have Avi@ and tommi@ listed as reviewers. Well my l-g-t-m is good for all the */media/ folders, which means you no longer need a stamp from tommi@. You need an l-g-t-m from avi@ for content/public/* and content/content_resources.grd Typical course of action is: 1) Is the person on vacation? Check their calendar. 2) If not, publish+mail and explicitly list their name + what you need them to review For example: avi: OWNERS review for content/public/* and content/content_resources.grd
On 2013/07/19 19:34:57, scherkus wrote: > On 2013/07/19 19:25:42, Ty Overby wrote: > > On 2013/07/19 19:21:28, scherkus wrote: > > > LGTM > > > > > > I believe you'll need some OWNERS reviews as well > > > > How do I get those again? I already have Avi@ and tommi@ listed as reviewers. > > Well my l-g-t-m is good for all the */media/ folders, which means you no longer > need a stamp from tommi@. > > You need an l-g-t-m from avi@ for content/public/* and > content/content_resources.grd > > Typical course of action is: > 1) Is the person on vacation? Check their calendar. > 2) If not, publish+mail and explicitly list their name + what you need them to > review > > For example: > avi: OWNERS review for content/public/* and content/content_resources.grd creis: OWNERS review for content/public/* and content/content_resources.grd
LGTM with nits. https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... content/public/common/content_switches.cc:411: // https://code.google.com/p/chromium/issues/detail?id=260005 This link is fine, but it's more common to use the shortcut URL in comments: http://crbug.com/260005. https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... File content/public/common/content_switches.h (right): https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... content/public/common/content_switches.h:141: extern const char kEnableNewMediaInternals[]; nit: Alphabetize.
https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... content/public/common/content_switches.cc:411: // https://code.google.com/p/chromium/issues/detail?id=260005 On 2013/07/19 19:55:14, creis wrote: > This link is fine, but it's more common to use the shortcut URL in comments: > http://crbug.com/260005. Done. https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... File content/public/common/content_switches.h (right): https://codereview.chromium.org/19722011/diff/18001/content/public/common/con... content/public/common/content_switches.h:141: extern const char kEnableNewMediaInternals[]; On 2013/07/19 19:55:14, creis wrote: > nit: Alphabetize. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/19722011/28001
https://codereview.chromium.org/19722011/diff/28001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/19722011/diff/28001/content/public/common/con... content/public/common/content_switches.cc:412: const char kEnableNewMediaInternals[] = "enable-new-media-internals"; This still isn't alphabetized. It should be in the same order as the .h file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/19722011/46001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/19722011/46001
Message was sent while issue was closed.
Change committed as 212760 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
