692829 - Display cached messages from iframes

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Mihai Sucan [:msucan]]

Once bugs 611032 and 609890 land, the Web Console will only show errors and messages from the top level window of the page. We need to also display the cached messages coming from iframes.

Comment 1

[Dave Camp (:dcamp)]

We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

Comment 3

[Dave Camp (:dcamp)]

This feels higher than P3 to me.  It's making test development annoying for me...

Comment 4

[Rob Campbell [:rc] (:robcee)]

yeah, we should do better than this. Taking it for investigation.

Comment 5

[Mihai Sucan [:msucan]]

Agreed.

Rob, what needs to be done here is to recursively iterate through the iframes of the top level window, get their inner window IDs, and use those to match which console API messages and script errors we get from the caches. This code lives in WebConsoleUtils.jsm, see ConsoleServiceListener/PageErrorListener and ConsoleAPIListener - they both have a getCachedMessages() method.

Thank you!

Comment 6

[Rob Campbell [:rc] (:robcee)]

thanks for the run-down. I'll take a look.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Attachment: WIP, tested locally, needs unittest

Comment 8

[Mihai Sucan [:msucan]]

rob++! that looks good!

Comment 9

[Rob Campbell [:rc] (:robcee)]

Attachment: v1, needs test

tested locally, fixed failures.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Attachment: v1.1

Comment 11

[Rob Campbell [:rc] (:robcee)]

Attachment: v1.2

removed some extra testing gunk.

Comment 12

[Rob Campbell [:rc] (:robcee)]

try:

https://tbpl.mozilla.org/?tree=Try&rev=f310fa087f30

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 766794 [details] [diff] [review]
v1.2

Review of attachment 766794 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Patch looks good. Minor comments below.

::: browser/devtools/webconsole/test/browser_console_iframe_messages.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Check that nested iframe nsIConsoleMessages are displayed in the Web Console.

Check that cached messages from nested iframes...

(caching is the important part, I think)

@@ +42,5 @@
> +    ],
> +  }).then(() => {
> +    let text = hud.outputNode.textContent;
> +    ok(text.contains("iframe 1"),
> +       "nsIConsoleMessages are not displayed for iframes");

not displayed?

Also, why not put "iframe 1", "iframe 2" and "iframe 3" up in the messages array for the waitForMessages() call? Also, you can add count: 2, for "iframe 1" to make sure that message shows twice.

@@ +83,5 @@
> +       "nsIConsoleMessages are not displayed for iframes in browser console");
> +    ok(text.contains("iframe 2"),
> +       "nsIConsoleMessages are not displayed for iframes (confirmed) in browser console");
> +    is(text.match(/iframe 1/g).length, 2,
> +       "nested iframe console messages not displayed in browser console");

Same as above.

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +148,3 @@
>    },
>  
> +  getInnerWindowIDsForFrames: function WCU_getInnerWindowIDsForFrames(aWindow)

Maybe we should add a js comment for this new function.

@@ +149,5 @@
>  
> +  getInnerWindowIDsForFrames: function WCU_getInnerWindowIDsForFrames(aWindow)
> +  {
> +    if (!aWindow)
> +      return [];

Do we need this? It seems to me that callers should not invoke this method if there's no |window|.

@@ +1158,4 @@
>     */
>    getCachedMessages: function CAL_getCachedMessages(aIncludePrivate = false)
>    {
> +    let ids = WebConsoleUtils.getInnerWindowIDsForFrames(this.window);

Maybe we should do this only if this.window is available.

Comment 14

[Rob Campbell [:rc] (:robcee)]

alright.

Comment 15

[Rob Campbell [:rc] (:robcee)]

ps, thanks for the review. :)

Comment 16

[Rob Campbell [:rc] (:robcee)]

Attachment: v1.3

comments addressed.

Comment 17

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/83e5f507582c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83e5f507582c