Integrate ErrorConsole with Apps Dev Tool

Integrate ErrorConsole with Apps Dev Tool

This is in preparation for integrating the error console work with the Apps Developer Tool.  On the chrome side, this means:
- Adding new DeveloperPrivateAPI functions to open developer tools and read file source.
- Making DeveloperPrivateAPI listen for extension errors.
- Refactor ExtensionErrorHandler WebUI to allow for shared code with DeveloperPrivateAPI (error_handler_ui_util).
- Refactor extension_error[_overlay].js to use either chrome.send or chrome.developerPrivate functions.
- Refactor so that all knowledge of whether the overlay can be displayed is in extension_error_overlay.js

Images: http://imgur.com/a/IAGMU#0
BUG=335313
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250943

[Devlin, 2014-02-10 17:49:35]

Please take a look when you can:
- dvh: c/b/e/api/developer_private (I know you're not OWNER, but you've done
most of the work here, so I wanna run it by you).
- finnur: c/b/extensions, c/c/extensions, c/b/ui/webui/extensions/
- dbeam: c/b/r/extensions

Thanks in advance!

[dvh, 2014-02-10 20:04:03]

lgtm.
Try to clean the GetStrings in developer private.

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1389:
IDS_APPS_DEVTOOL_MANAGED_PROFILE_DIALOG_DESCRIPTION);
I think that the strings above are probably deprecated.
You can probably clean them up.

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1394:
base::Bind(&SetString, base::Unretained(dict)));
What is it used for?

--
I went through all the review. I think DeveloperPrivateGetStringsFunction is
deprecated now and can be removed. Apps Developer Tools does not rely on it but
on its embedded strings.

[Devlin, 2014-02-10 22:59:41]

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1389:
IDS_APPS_DEVTOOL_MANAGED_PROFILE_DIALOG_DESCRIPTION);
On 2014/02/10 20:04:04, dvh wrote:
> I think that the strings above are probably deprecated.
> You can probably clean them up.

Done.

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1394:
base::Bind(&SetString, base::Unretained(dict)));
On 2014/02/10 20:04:04, dvh wrote:
> What is it used for?
> 
> --
> I went through all the review. I think DeveloperPrivateGetStringsFunction is
> deprecated now and can be removed. Apps Developer Tools does not rely on it
but
> on its embedded strings.

Woo!  That makes it a little cleaner.  Done.

[Dan Beam, 2014-02-11 02:43:48]

https://chromiumcodereview.appspot.com/150663013/diff/60001/chrome/browser/ex...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://chromiumcodereview.appspot.com/150663013/diff/60001/chrome/browser/ex...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:386:
developer::InstallWarning* warning = new developer::InstallWarning();
nit: arguably make this a scoped_ptr and .release() as I've seen multiple leaks
from doing this:

  for (size_t i = 0; i < leakers; ++i) {
    Thinger* leaky  = new Thinger;
    if (i % 2 == 0)
      continue;  // or i == 3, break; or something
    other_object->Take(leaky);
  }

where if you break/continue in the loop the memory is semi-sneakily leaked.

https://chromiumcodereview.appspot.com/150663013/diff/60001/chrome/browser/ex...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1411:
DeveloperPrivateRequestFileSourceFunction() {}
nit: indent by 4\s to show it's a continuation

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:71:
console.error('Cannot call either openDevTools function.');
when do you expect this to ever happen?

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:72: }
no curlies

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:185: ':' +
frame.lineNumber;
nit: arguably, revert the style difference

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:287: return
RegExp('^' + extensionUrl).test(file) ||
nit: arguably just re-use the .substr(0, extensionUrl.length) = extensionUrl

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:327:
extensions.ExtensionErrorOverlay.requestFileSourceResponse(result);
-2\s

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:330:
console.error('Cannot call either requestFileSource function.');
why will this happen?

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:346: * @param
{function} showOverlay The function to show or hide the
@param {function(HTMLDivElement)} showOverlay

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:374: };
please move this out of the ctor

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/r...
chrome/browser/resources/extensions/extension_error_overlay.js:394: 
to somewhere like here:

  /* ... */
  setVisible: function(...) {
    ...
  },

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:23: void
CallJavascriptFunction(content::WebUI* ui,
nit: s/ui/web_ui/

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:107:
"extensions.ExtensionErrorOverlay.requestFileSourceResponse"));
^ i think this is clearer if not bound, i.e.

in .h:

   private:
    // Called when |error_ui_util::HandleRequestFileSource| succeeds.
    void OnFileSourceHandled(const base::DictionaryValue& files);

in .cc, call site

    error_ui_util::HandleRequestFileSource(...
        base::Bind(&ExtensionErrorHandler::OnFiledSourceHandled));

in .cc, implementation

  ExtensionErrorHandler::OnFiledSourceHandled(
      const base::DictionaryValue& files) {
    web_ui()->CallJavascriptFunction(
        "extensions.ExtensionErrorOverlay.requestFileSourceResponse", files);
  }

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
File chrome/browser/ui/webui/extensions/extension_error_ui_util.cc (right):

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:40:
base::ReadFileToString(path, &data);
i have a feeling this method uses |data| as an out-param because it requires
less copying -- why are you undoing this?

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:81:
ExtensionSystem::Get(profile)->extension_service()->GetExtensionById(
^ what if |extension_service()| is NULL?

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:182:
tab_strip->ActivateTabAt(tab_strip->GetIndexOfWebContents(web_contents),
^ what if !browser->is_type_tabbed() (i.e. it's a popup or an app?)

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:183: false);  //
Not through direct user gesture.
^ didn't the user click "Open dev tools"?

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
File chrome/browser/ui/webui/extensions/extension_error_ui_util.h (right):

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:11: #include
"base/strings/string16.h"
move both string and string16 includes to .cc

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:23:
JavascriptResponseCallback;
^ is this used outside this file?

https://chromiumcodereview.appspot.com/150663013/diff/160001/chrome/browser/u...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:24: 
doc comments

[Finnur, 2014-02-11 13:11:28]

General comment: 
You'll need one of: asvitkine, isherman, jar, mpearson to give OWNERS approval
for histograms.xml.

Otherwise, LGTM with nits:

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:161: :
profile_(profile) {
nit: Indentation is off here.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:364: }
else {  // runtime error
nit: End all regular comments with period.

More important, though, I would prefer:

switch ((*iter)->type()) {
  case ExtensionError::MANIFEST_ERROR: ... etc ...
  case ExtensionError::RUNTIME_ERROR:  ... etc ...
}

... over an if-else, so that if someone adds to the enum you won't have a bug
here. That goes double since you static_cast *iter in the else clause.

Oh, and make sure to *not* provide any default: case, to ensure the person who
adds the enum gets a compile failure here (on Linux and Mac at least).

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.h:47: }
nit: // namespace developer_private

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.h:82: //
since we may treat events with these extensions differently.
nit: This comment leaves more questions than it answers... :)

https://codereview.chromium.org/150663013/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/common/extension...
chrome/common/extensions/api/developer_private.idl:205: static void
openDevTools(any dict);
Document.

[Devlin, 2014-02-11 18:41:16]

Finnur's and Dan's comments addressed.

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:386:
developer::InstallWarning* warning = new developer::InstallWarning();
On 2014/02/11 02:43:49, Dan Beam wrote:
> nit: arguably make this a scoped_ptr and .release() as I've seen multiple
leaks
> from doing this:
> 
>   for (size_t i = 0; i < leakers; ++i) {
>     Thinger* leaky  = new Thinger;
>     if (i % 2 == 0)
>       continue;  // or i == 3, break; or something
>     other_object->Take(leaky);
>   }
> 
> where if you break/continue in the loop the memory is semi-sneakily leaked.

Done.

https://codereview.chromium.org/150663013/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1411:
DeveloperPrivateRequestFileSourceFunction() {}
On 2014/02/11 02:43:49, Dan Beam wrote:
> nit: indent by 4\s to show it's a continuation

Done in Patch Set 2.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:161: :
profile_(profile) {
On 2014/02/11 13:11:28, Finnur wrote:
> nit: Indentation is off here.

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:364: }
else {  // runtime error
On 2014/02/11 13:11:28, Finnur wrote:
> nit: End all regular comments with period.
> 
> More important, though, I would prefer:
> 
> switch ((*iter)->type()) {
>   case ExtensionError::MANIFEST_ERROR: ... etc ...
>   case ExtensionError::RUNTIME_ERROR:  ... etc ...
> }
> 
> ... over an if-else, so that if someone adds to the enum you won't have a bug
> here. That goes double since you static_cast *iter in the else clause.
> 
> Oh, and make sure to *not* provide any default: case, to ensure the person who
> adds the enum gets a compile failure here (on Linux and Mac at least).

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.h:47: }
On 2014/02/11 13:11:28, Finnur wrote:
> nit: // namespace developer_private

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.h:82: //
since we may treat events with these extensions differently.
On 2014/02/11 13:11:28, Finnur wrote:
> nit: This comment leaves more questions than it answers... :)

Fair point. :)  Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:71:
console.error('Cannot call either openDevTools function.');
On 2014/02/11 02:43:49, Dan Beam wrote:
> when do you expect this to ever happen?

Ideally, _never_.  Would it be best to remove it, assert instead, or...?  (I'm
not sure how we usually spell NOTREACHED() in JS...)

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:72: }
On 2014/02/11 02:43:49, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:185: ':' +
frame.lineNumber;
On 2014/02/11 02:43:49, Dan Beam wrote:
> nit: arguably, revert the style difference

Doesn't fit on one line anymore (needs the new parameter |extensionUrl|). :/

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:287: return
RegExp('^' + extensionUrl).test(file) ||
On 2014/02/11 02:43:49, Dan Beam wrote:
> nit: arguably just re-use the .substr(0, extensionUrl.length) = extensionUrl

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:327:
extensions.ExtensionErrorOverlay.requestFileSourceResponse(result);
On 2014/02/11 02:43:49, Dan Beam wrote:
> -2\s

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:330:
console.error('Cannot call either requestFileSource function.');
On 2014/02/11 02:43:49, Dan Beam wrote:
> why will this happen?

Again, ideally never (see above).

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:346: * @param
{function} showOverlay The function to show or hide the
On 2014/02/11 02:43:49, Dan Beam wrote:
> @param {function(HTMLDivElement)} showOverlay

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:374: };
On 2014/02/11 02:43:49, Dan Beam wrote:
> please move this out of the ctor

This uses the showOverlay parameter passed into the ctor, so we'd have to set
this.showOverlay_ = showOverlay and then use it in setVisible, so I'm not sure
this would make it any cleaner...? (If it does, happy to change it, just want to
make sure you saw the reason it was in here to begin.)

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:394: 
On 2014/02/11 02:43:49, Dan Beam wrote:
> to somewhere like here:
> 
>   /* ... */
>   setVisible: function(...) {
>     ...
>   },

See above.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_ui_util.cc (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:40:
base::ReadFileToString(path, &data);
On 2014/02/11 02:43:49, Dan Beam wrote:
> i have a feeling this method uses |data| as an out-param because it requires
> less copying -- why are you undoing this?

base::ReadFileToString() actually returns a bool indicating the success, but, in
this case, if we fail, we don't really care, because we handle empty strings. 
This way, we can do the far more elegant PostTaskAndReply() at line 127, rather
than having a PostTask() with a manual reply, or using owned ptrs (which get
hairy in threads).  Also, with RVO, it actually shouldn't make a difference
(ReadFileToString() uses |data| as an out-param so it can return a bool, not for
speed purposes).

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:81:
ExtensionSystem::Get(profile)->extension_service()->GetExtensionById(
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ what if |extension_service()| is NULL?

Hmm... I wonder if this can happen (this is only called by the chrome:extensions
page, which very much requires ExtensionService to be alive).  But, potentially,
if everything was closing down, maybe... Added a graceful handling to be safe.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:182:
tab_strip->ActivateTabAt(tab_strip->GetIndexOfWebContents(web_contents),
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ what if !browser->is_type_tabbed() (i.e. it's a popup or an app?)

It actually works fine now - for Browsers::TYPE_POPUP,
FindBrowserWithWebContents() returns null, DevTools opens in a new window, and
we return early.  But just in case that's actually a bug in FindBrowser...(), I
updated the comment and added a check to make sure it's tabbed.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.cc:183: false);  //
Not through direct user gesture.
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ didn't the user click "Open dev tools"?

Yeah, kinda anti-intuitive.

"|user_gesture| is true if the user actually clicked on the tab or navigated to
it using a keyboard command, false if the tab was activated as a by-product of
some other action."

I think this more falls into the "by-product of some other action" (since they
didn't click the tab, and I think the "keyboard commands" referenced would be
like Ctrl+2).

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/150663013/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/common/extension...
chrome/common/extensions/api/developer_private.idl:205: static void
openDevTools(any dict);
On 2014/02/11 13:11:28, Finnur wrote:
> Document.

Whoops! Done.

[Dan Beam, 2014-02-11 18:54:43]

still a few more things to reconcile

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:107:
"extensions.ExtensionErrorOverlay.requestFileSourceResponse"));
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ i think this is clearer if not bound, i.e.
> 
> in .h:
> 
>    private:
>     // Called when |error_ui_util::HandleRequestFileSource| succeeds.
>     void OnFileSourceHandled(const base::DictionaryValue& files);
> 
> in .cc, call site
> 
>     error_ui_util::HandleRequestFileSource(...
>         base::Bind(&ExtensionErrorHandler::OnFiledSourceHandled));
> 
> in .cc, implementation
> 
>   ExtensionErrorHandler::OnFiledSourceHandled(
>       const base::DictionaryValue& files) {
>     web_ui()->CallJavascriptFunction(
>         "extensions.ExtensionErrorOverlay.requestFileSourceResponse", files);
>   }

what about this?

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_ui_util.h (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:11: #include
"base/strings/string16.h"
On 2014/02/11 02:43:49, Dan Beam wrote:
> move both string and string16 includes to .cc

^ and this

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:23:
JavascriptResponseCallback;
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ is this used outside this file?

and this

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:24: 
On 2014/02/11 02:43:49, Dan Beam wrote:
> doc comments

and this

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.h:435: //
ExtensionFunction
nit:

  // ExtensionFunction:

or

  // ExtensionFunction implementation.

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:71:
console.error('Cannot call either openDevTools function.');
assert(false, 'is the NOTREACHED() of JS');

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:380: },
};

[Devlin, 2014-02-11 19:26:44]

Dang, don't know how I didn't see those comments. :/  Sorry...

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:23: void
CallJavascriptFunction(content::WebUI* ui,
On 2014/02/11 02:43:49, Dan Beam wrote:
> nit: s/ui/web_ui/

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:107:
"extensions.ExtensionErrorOverlay.requestFileSourceResponse"));
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ i think this is clearer if not bound, i.e.
> 
> in .h:
> 
>    private:
>     // Called when |error_ui_util::HandleRequestFileSource| succeeds.
>     void OnFileSourceHandled(const base::DictionaryValue& files);
> 
> in .cc, call site
> 
>     error_ui_util::HandleRequestFileSource(...
>         base::Bind(&ExtensionErrorHandler::OnFiledSourceHandled));
> 
> in .cc, implementation
> 
>   ExtensionErrorHandler::OnFiledSourceHandled(
>       const base::DictionaryValue& files) {
>     web_ui()->CallJavascriptFunction(
>         "extensions.ExtensionErrorOverlay.requestFileSourceResponse", files);
>   }

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_ui_util.h (right):

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:11: #include
"base/strings/string16.h"
On 2014/02/11 02:43:49, Dan Beam wrote:
> move both string and string16 includes to .cc

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:11: #include
"base/strings/string16.h"
On 2014/02/11 02:43:49, Dan Beam wrote:
> move both string and string16 includes to .cc

Done.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:23:
JavascriptResponseCallback;
On 2014/02/11 02:43:49, Dan Beam wrote:
> ^ is this used outside this file?

No, but I've been told before that doing this improves readability, even in the
function declaration.  Renamed it to RequestFileSourceCallback to be slightly
more accurate.  Of course, it's always OWNERS preference, so if you'd like me to
remove the typedef so we don't have to include callback.h here, I'm happy to.

https://codereview.chromium.org/150663013/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_ui_util.h:24: 
On 2014/02/11 02:43:49, Dan Beam wrote:
> doc comments

Done.

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.h:435: //
ExtensionFunction
On 2014/02/11 18:54:44, Dan Beam wrote:
> nit:
> 
>   // ExtensionFunction:
> 
> or
> 
>   // ExtensionFunction implementation.

Whoops, done.

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_error_overlay.js (right):

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:71:
console.error('Cannot call either openDevTools function.');
On 2014/02/11 18:54:44, Dan Beam wrote:
> assert(false, 'is the NOTREACHED() of JS');

ah, okay. Done. :)

https://codereview.chromium.org/150663013/diff/370001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_error_overlay.js:380: },
On 2014/02/11 18:54:44, Dan Beam wrote:
> };

D'oh. Done.

[Dan Beam, 2014-02-11 19:54:20]

https://codereview.chromium.org/150663013/diff/590001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/150663013/diff/590001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:96:
base::Unretained(this)));
^ can this handler go away while request is processing (e.g. page closes)?  if
so, you should probably use a WeakPtrFactory.

[Devlin, 2014-02-11 20:15:01]

https://codereview.chromium.org/150663013/diff/590001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right):

https://codereview.chromium.org/150663013/diff/590001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_error_handler.cc:96:
base::Unretained(this)));
On 2014/02/11 19:54:21, Dan Beam wrote:
> ^ can this handler go away while request is processing (e.g. page closes)?  if
> so, you should probably use a WeakPtrFactory.

Done.

[Dan Beam, 2014-02-11 20:25:30]

lgtm thanks!

[Devlin, 2014-02-11 21:08:13]

+mpearson for histograms

[Mark P, 2014-02-11 22:08:36]

histograms.xml lgtm

[Devlin, 2014-02-12 00:23:44]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-12 00:26:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/150663013/...

[commit-bot: I haz the power, 2014-02-12 06:01:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 06:01:50]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Devlin, 2014-02-12 16:59:23]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-12 17:01:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/150663013/...

[commit-bot: I haz the power, 2014-02-12 17:56:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 17:56:59]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 741.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
affba3dd301c4ba610afc1a66b02c20cb7422f23..2564158a6bb65bd4060a151ce03f6ef25c8c66af
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -741,6 +741,8 @@ enum HistogramValue {
   HOTWORDPRIVATE_GETSTATUS,
   APP_CURRENTWINDOWINTERNAL_SETBADGEICON,
   APP_CURRENTWINDOWINTERNAL_CLEARBADGE,
+  DEVELOPERPRIVATE_REQUESTFILESOURCE,
+  DEVELOPERPRIVATE_OPENDEVTOOLS,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[Mark P, 2014-02-12 18:01:20]

When you resolve to fix this chunk mismatch, please remember to update
histograms.xml appropriately too.

[Devlin, 2014-02-12 18:07:14]

On 2014/02/12 18:01:20, Mark P wrote:
> When you resolve to fix this chunk mismatch, please remember to update
> histograms.xml appropriately too.

Looks like the chunk mismatch was only in extension_function_histogram_value.h,
and no new entries were added to histograms.xml, so it's still okay.  Right?

[Devlin, 2014-02-12 18:36:51]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-12 18:38:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/150663013/...

[Mark P, 2014-02-12 18:38:35]

On 2014/02/12 18:07:14, D Cronin wrote:
> On 2014/02/12 18:01:20, Mark P wrote:
> > When you resolve to fix this chunk mismatch, please remember to update
> > histograms.xml appropriately too.
> 
> Looks like the chunk mismatch was only in
extension_function_histogram_value.h,
> and no new entries were added to histograms.xml, so it's still okay.  Right?

I don't think so.  I think the enum values you are adding to histograms.xml no
longer have the values you list in histograms.xml.  Am I mistaken?

--mark

[Devlin, 2014-02-12 18:55:51]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2014-02-12 18:57:19]

On 2014/02/12 18:38:35, Mark P wrote:
> On 2014/02/12 18:07:14, D Cronin wrote:
> > On 2014/02/12 18:01:20, Mark P wrote:
> > > When you resolve to fix this chunk mismatch, please remember to update
> > > histograms.xml appropriately too.
> > 
> > Looks like the chunk mismatch was only in
> extension_function_histogram_value.h,
> > and no new entries were added to histograms.xml, so it's still okay.  Right?
> 
> I don't think so.  I think the enum values you are adding to histograms.xml no
> longer have the values you list in histograms.xml.  Am I mistaken?
> 
> --mark

Hmm... it looks like the last ~10 or so functions people added to
extension_function_histogram_value.h never made it into histograms.xml. Uploaded
a new patch with the updated histograms.xml; this look better?

[Mark P, 2014-02-12 19:03:22]

On Wed, Feb 12, 2014 at 10:57 AM, <rdevlin.cronin@chromium.org> wrote:

> On 2014/02/12 18:38:35, Mark P wrote:
>
>> On 2014/02/12 18:07:14, D Cronin wrote:
>> > On 2014/02/12 18:01:20, Mark P wrote:
>> > > When you resolve to fix this chunk mismatch, please remember to update
>> > > histograms.xml appropriately too.
>> >
>> > Looks like the chunk mismatch was only in
>> extension_function_histogram_value.h,
>> > and no new entries were added to histograms.xml, so it's still okay.
>>  Right?
>>
>
>  I don't think so.  I think the enum values you are adding to
>> histograms.xml no
>> longer have the values you list in histograms.xml.  Am I mistaken?
>>
>
>  --mark
>>
>
> Hmm... it looks like the last ~10 or so functions people added to
> extension_function_histogram_value.h never made it into histograms.xml.
> Uploaded
> a new patch with the updated histograms.xml; this look better?
>

Yes, that looks better.

Can you do a quick printf of the final enum DEVELOPERPRIVATE_OPENDEVTOOLS
just to make sure it's 685 as you'd expect and as you claim in
histograms.xml?

If so, then lgtm.

--mark


>
> https://codereview.chromium.org/150663013/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2014-02-12 19:32:15]

On 2014/02/12 19:03:22, Mark P wrote:
> On Wed, Feb 12, 2014 at 10:57 AM, <mailto:rdevlin.cronin@chromium.org> wrote:
> 
> > On 2014/02/12 18:38:35, Mark P wrote:
> >
> >> On 2014/02/12 18:07:14, D Cronin wrote:
> >> > On 2014/02/12 18:01:20, Mark P wrote:
> >> > > When you resolve to fix this chunk mismatch, please remember to update
> >> > > histograms.xml appropriately too.
> >> >
> >> > Looks like the chunk mismatch was only in
> >> extension_function_histogram_value.h,
> >> > and no new entries were added to histograms.xml, so it's still okay.
> >>  Right?
> >>
> >
> >  I don't think so.  I think the enum values you are adding to
> >> histograms.xml no
> >> longer have the values you list in histograms.xml.  Am I mistaken?
> >>
> >
> >  --mark
> >>
> >
> > Hmm... it looks like the last ~10 or so functions people added to
> > extension_function_histogram_value.h never made it into histograms.xml.
> > Uploaded
> > a new patch with the updated histograms.xml; this look better?
> >
> 
> Yes, that looks better.
> 
> Can you do a quick printf of the final enum DEVELOPERPRIVATE_OPENDEVTOOLS
> just to make sure it's 685 as you'd expect and as you claim in
> histograms.xml?
> 
> If so, then lgtm.
> 
> --mark
> 
> 
> >
> > https://codereview.chromium.org/150663013/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Checked, it is.  Thanks for the diligence. :)

[Devlin, 2014-02-12 19:33:10]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:49:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/150663013/...

[commit-bot: I haz the power, 2014-02-13 04:23:27]

Change committed as 250943