Bug 103255

Created attachment 176003 [details] Patch

Comment on attachment 176003 [details] Patch I'm not opposed to this, but I'd strongly prefer a UI that shows the error in the place it happened (e.g. inline in the html of the page and/or next to the DOM node in the elements panel). It's too easy to end up with a bunch of spam in the console that makes the console warnings useless.

Perhaps this and bug 102750 could use the same solution.

(In reply to comment #2) > (From update of attachment 176003 [details]) > I'm not opposed to this, but I'd strongly prefer a UI that shows the error in the place it happened (e.g. inline in the html of the page and/or next to the DOM node in the elements panel). It's too easy to end up with a bunch of spam in the console that makes the console warnings useless. 1. Once bug 100650 lands, this message would have a line number and url associated with it, which means you could jump directly to the place where the error occurred. 2. I agree with the larger point, though. It would be very nice indeed to have a separate location for errors like this, which are conceptually distinct from the general console messages that a developer might throw to herself. Given that that idea has been floating around for quite some time, though, I'd rather err towards giving the developer too much information, rather than just blocking some action and not explaining why. This bug is a poor example, of course. A better sandboxing example would be something more obscure, like a plugin not loading in a sandboxed iframe. Currently, that's a black box.

I'm OK with moving forward with this patch as is with a FIXME + bug that we should add a UI specifically for audit style warnings so that they don't spam the console.

Thanks Ojan. I've filed bug 103274 to continue the conversation about a UI for these sorts of warnings.

Created attachment 176256 [details] Patch

Comment on attachment 176256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176256&action=review > LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:6 > +<iframe sandbox > + src="data:text/html,<input autofocus onfocus>"></iframe> I would have put this in the sandbox test suite.

I'll let Ojan review this patch since he seems concerned about spamming the console, which is the main consideration here.

Comment on attachment 176256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176256&action=review The test needs a bit of work, but this looks good to me otherwise. > LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:4 > +</script> Would be good to have a textual description of what this test is testing so people can know if the output is correct. >> LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:6 >> + src="data:text/html,<input autofocus onfocus>"></iframe> > > I would have put this in the sandbox test suite. And no need to wrap this line.

(In reply to comment #10) > (From update of attachment 176256 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176256&action=review > > The test needs a bit of work, but this looks good to me otherwise. > > > LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:4 > > +</script> > > Would be good to have a textual description of what this test is testing so people can know if the output is correct. > > >> LayoutTests/fast/forms/autofocus-in-sandbox-without-allow-scripts.html:6 > >> + src="data:text/html,<input autofocus onfocus>"></iframe> > > > > I would have put this in the sandbox test suite. > > And no need to wrap this line. Ok. Just so I'm clear, 'fast/forms/autofocus-in-sandbox.html' is good where it is, as it's really testing autofocus. This test is really testing sandboxing (or a side-effect thereof), so it's better placed along with other sandbox tests. Is that correct?

(In reply to comment #11) > Ok. Just so I'm clear, 'fast/forms/autofocus-in-sandbox.html' is good where it is, as it's really testing autofocus. This test is really testing sandboxing (or a side-effect thereof), so it's better placed along with other sandbox tests. Is that correct? 'fast/frames/sandboxed-iframe-*' is better, I suppose.

I don't have a preference here. I defer to Adam. :)

Created attachment 176313 [details] Patch

(In reply to comment #10) > (From update of attachment 176256 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176256&action=review > > The test needs a bit of work, but this looks good to me otherwise. You were right, I rushed it. The latest patch's test is much less of a copout. :) WDYT?

Comment on attachment 176313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176313&action=review > LayoutTests/fast/frames/sandboxed-iframe-autofocus-denied.html:1 > +<!doctype html> s/doctype/DOCTYPE

Created attachment 176317 [details] Patch for landing

(In reply to comment #16) > (From update of attachment 176313 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176313&action=review > > > LayoutTests/fast/frames/sandboxed-iframe-autofocus-denied.html:1 > > +<!doctype html> > > s/doctype/DOCTYPE Done, thanks!

Comment on attachment 176317 [details] Patch for landing Clearing flags on attachment: 176317 Committed r135903: <http://trac.webkit.org/changeset/135903>

All reviewed patches have been landed. Closing bug.