Description was changed from ========== somefin BUG= ========== to ========== somefin BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
3 years, 9 months ago
(2017-03-23 21:00:57 UTC)
#1
Description was changed from
==========
somefin
BUG=
==========
to
==========
somefin
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
Devlin
Description was changed from ========== somefin BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Extensions] Include the ...
3 years, 9 months ago
(2017-03-23 22:21:37 UTC)
#2
Description was changed from
==========
somefin
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
[MD Extensions] Include the manifest code snippet in load errors
Add the manifest code snippet to errors displayed when loading an
unpacked extension fails. Add tests for the same. Update Service to
display the load error dialog when a load error occurs.
BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
3 years, 9 months ago
(2017-03-23 22:21:50 UTC)
#4
Michael, mind taking a look?
michaelpg
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/code_section.js File chrome/browser/resources/md_extensions/code_section.js (right): https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/code_section.js#newcode27 chrome/browser/resources/md_extensions/code_section.js:27: noCodeError: String, I might just be silly, but I'm ...
3 years, 9 months ago
(2017-03-24 02:59:02 UTC)
#5
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/code_section.js File chrome/browser/resources/md_extensions/code_section.js (right): https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/code_section.js#newcode27 chrome/browser/resources/md_extensions/code_section.js:27: noCodeError: String, On 2017/03/24 02:59:01, michaelpg wrote: > I ...
3 years, 9 months ago
(2017-03-24 16:03:08 UTC)
#6
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/code_section.js (right):
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/code_section.js:27: noCodeError: String,
On 2017/03/24 02:59:01, michaelpg wrote:
> I might just be silly, but I'm not sure which of these "noCodeError" means:
>
> * "error: no code!" (an error we've encountered that says we couldn't show the
> code for some unexpected reason), or
> * "no, not a code error!" (a boolean saying whether or not there is a code
error
> to show the extension developer), or
> * "message for a non-code-based error" (a string to show when the developer's
> extension has an error that doesn't come from their code, i.e. when it's a
load
> error)
>
> I suspect it's the last one, so a more accurate string would be
> "errorTextToShowWhenTheErrorThisUIIsForIsNotInTheCode" but pithier. But then
I'd
> wonder if a <code-section> element is the right choice for a generic error --
> one that doesn't involve a section of code.
>
> Feel free to swing by my desk tmrw if what I just said makes no sense. It's
been
> a long day...
It's actually the first - we fail to load the relevant source file. Renamed to
couldNotDisplayCode and clarified in the comment.
Even for load errors, it's usually a 'code' error. e.g. if an extension
manifest is invalid json (like missing a comma or an end quote), the error is
"manifest is invalid json, line 10, column 3, syntax error", and we'll load up
the manifest and display it, highlighting the error spot. The only time we
*don't* load a file is if, for instance, the error is that the file itself is
missing (like not having a manifest.json file), in which case we show "Could not
load manifest.json".
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/code_section.js:51: * Returns true if no
code could be displayed.
On 2017/03/24 02:59:01, michaelpg wrote:
> As above, I'm asking myself *why* the code couldn't be displayed.
added "e.g. because the file could not be loaded."
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/code_section.js:54: isEmpty: function() {
On 2017/03/24 02:59:01, michaelpg wrote:
> nit: move above @private method
Done.
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/compiled_resources2.gyp (right):
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/compiled_resources2.gyp:121:
'code_section',
On 2017/03/24 02:59:01, michaelpg wrote:
> not actually used, I think?
It's used in spirit? ;)
// load_error.js
this.$.code.set('code', codeSectionProperties);
// load_error.html
<extensions-code-section id="code"></extensions-code-section>
So, conceptually, we *are* using it - we're reaching into the object and setting
the 'code' property on it. I don't think closure is quite smart enough to
realize that yet, and I didn't *really* want to do the ugly cast of
(/** @type {extensions.CodeSection} */(this.$.code)).set(), since I also don't
think closure validates set() calls in any way.
WDYT in this situation? Include it for completeness, or omit it because it
doesn't technically matter?
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/load_error.js (right):
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/load_error.js:51: var
codeSectionProperties = {
On 2017/03/24 02:59:01, michaelpg wrote:
> does closure let you "cast" this to @type !RequestFileSourceResponse? (so it
can
> yell at us if we massage this incorrectly)
Yep, done.
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/service.js (right):
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/service.js:257: {failQuietly: true,
populateError: true}, (loadError) => {
On 2017/03/24 02:59:02, michaelpg wrote:
> nit: start the lambda on its own line, so it stands out as a function and
passes
> the Rectangle Rule.
Done.
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/service.js:259: // The lastError here is
just that the user closed the dialog.
On 2017/03/24 02:59:02, michaelpg wrote:
> assert(lastError matches what you expect) in addition to (or instead of)
comment
Done.
michaelpg
lgtm https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/compiled_resources2.gyp File chrome/browser/resources/md_extensions/compiled_resources2.gyp (right): https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/compiled_resources2.gyp#newcode121 chrome/browser/resources/md_extensions/compiled_resources2.gyp:121: 'code_section', On 2017/03/24 16:03:08, Devlin wrote: > On ...
3 years, 9 months ago
(2017-03-24 21:25:32 UTC)
#7
lgtm
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/compiled_resources2.gyp (right):
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/compiled_resources2.gyp:121:
'code_section',
On 2017/03/24 16:03:08, Devlin wrote:
> On 2017/03/24 02:59:01, michaelpg wrote:
> > not actually used, I think?
>
> It's used in spirit? ;)
>
> // load_error.js
> this.$.code.set('code', codeSectionProperties);
>
> // load_error.html
> <extensions-code-section id="code"></extensions-code-section>
>
> So, conceptually, we *are* using it - we're reaching into the object and
setting
> the 'code' property on it.
Agreed, and that makes sense for HTML imports too. But Closure takes long enough
to run that I'd prefer keeping this small if possible.
> I don't think closure is quite smart enough to
> realize that yet,
No. Actually, Closure makes bad inferences here that can hide actual errors, but
we do use this.$.something everywhere anyway. Sometimes we cast, sometimes not.
> and I didn't *really* want to do the ugly cast of
> (/** @type {extensions.CodeSection} */(this.$.code)).set(), since I also don't
> think closure validates set() calls in any way.
Technically it does, but not in a very useful way; |value| is just {*}:
@param {(string|Array<(string|number)>)} path
@param {*} value
@param {Object=} root
But! We only have to use set() for deep sub-properties, not top-level
properties.
>
> WDYT in this situation? Include it for completeness, or omit it because it
> doesn't technically matter?
I prefer omit unless you actually use it.
Devlin
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-24 22:44:51 UTC)
#8
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/391189)
3 years, 9 months ago
(2017-03-24 23:27:09 UTC)
#11
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/compiled_resources2.gyp File chrome/browser/resources/md_extensions/compiled_resources2.gyp (right): https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resources/md_extensions/compiled_resources2.gyp#newcode121 chrome/browser/resources/md_extensions/compiled_resources2.gyp:121: 'code_section', On 2017/03/24 21:25:32, michaelpg wrote: > On 2017/03/24 ...
3 years, 9 months ago
(2017-03-25 01:58:00 UTC)
#14
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/compiled_resources2.gyp (right):
https://codereview.chromium.org/2769293002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/compiled_resources2.gyp:121:
'code_section',
On 2017/03/24 21:25:32, michaelpg wrote:
> On 2017/03/24 16:03:08, Devlin wrote:
> > On 2017/03/24 02:59:01, michaelpg wrote:
> > > not actually used, I think?
> >
> > It's used in spirit? ;)
> >
> > // load_error.js
> > this.$.code.set('code', codeSectionProperties);
> >
> > // load_error.html
> > <extensions-code-section id="code"></extensions-code-section>
> >
> > So, conceptually, we *are* using it - we're reaching into the object and
> setting
> > the 'code' property on it.
>
> Agreed, and that makes sense for HTML imports too. But Closure takes long
enough
> to run that I'd prefer keeping this small if possible.
>
> > I don't think closure is quite smart enough to
> > realize that yet,
>
> No. Actually, Closure makes bad inferences here that can hide actual errors,
but
> we do use this.$.something everywhere anyway. Sometimes we cast, sometimes
not.
>
> > and I didn't *really* want to do the ugly cast of
> > (/** @type {extensions.CodeSection} */(this.$.code)).set(), since I also
don't
> > think closure validates set() calls in any way.
>
> Technically it does, but not in a very useful way; |value| is just {*}:
> @param {(string|Array<(string|number)>)} path
> @param {*} value
> @param {Object=} root
>
> But! We only have to use set() for deep sub-properties, not top-level
> properties.
>
> >
> > WDYT in this situation? Include it for completeness, or omit it because it
> > doesn't technically matter?
>
> I prefer omit unless you actually use it.
Done.
Devlin
The CQ bit was unchecked by rdevlin.cronin@chromium.org
3 years, 9 months ago
(2017-03-25 01:58:05 UTC)
#15
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490407091128930, "parent_rev": "3f08f81806674e0e9882a09c6f97852b184628b5", "commit_rev": "5370ad38ca596fffee45ffff85a74fda142b5a07"}
3 years, 9 months ago
(2017-03-25 03:45:04 UTC)
#19
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490407091128930,
"parent_rev": "3f08f81806674e0e9882a09c6f97852b184628b5", "commit_rev":
"5370ad38ca596fffee45ffff85a74fda142b5a07"}
commit-bot: I haz the power
Description was changed from ========== [MD Extensions] Include the manifest code snippet in load errors ...
3 years, 9 months ago
(2017-03-25 03:45:50 UTC)
#20
Message was sent while issue was closed.
Description was changed from
==========
[MD Extensions] Include the manifest code snippet in load errors
Add the manifest code snippet to errors displayed when loading an
unpacked extension fails. Add tests for the same. Update Service to
display the load error dialog when a load error occurs.
BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
[MD Extensions] Include the manifest code snippet in load errors
Add the manifest code snippet to errors displayed when loading an
unpacked extension fails. Add tests for the same. Update Service to
display the load error dialog when a load error occurs.
BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2769293002
Cr-Commit-Position: refs/heads/master@{#459636}
Committed:
https://chromium.googlesource.com/chromium/src/+/5370ad38ca596fffee45ffff85a7...
==========
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5370ad38ca596fffee45ffff85a74fda142b5a07
3 years, 9 months ago
(2017-03-25 03:45:51 UTC)
#21
Issue 2769293002: [MD Extensions] Include the manifest code snippet in load errors
(Closed)
Created 3 years, 9 months ago by Devlin
Modified 3 years, 9 months ago
Reviewers: michaelpg
Base URL:
Comments: 16