Bug 53359 – Web Inspector: Allow the console to persist on page refresh or navigation

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Darth]

Right now the console clears out when a page navigation happens or page refreshes. Please add an option to enable persistent logs just like firebug does. Pretty much the same as one sees the "Preserve log on page navigation" on the network tab.

[Yury Semikhatsky]

(In reply to comment #0)
> Right now the console clears out when a page navigation happens or page refreshes. Please add an option to enable persistent logs just like firebug does. Pretty much the same as one sees the "Preserve log on page navigation" on the network tab.

The main problem is that we cannot access JS objects from the page after navigation and if there are some references to them in the log records we wouldn't be able to expand such objects and the best we can do is to serialize the to strings right before navigation.

[Darth]

Yeah a toString version would be fine. Atleast one would be able to see the simple console.log() statements that one can use for debugging with the console persisting. And perhaps if we have xhr logging enabled it can still be a link to point to it's corresponding counterpart on the net tab if it's persist option was enabled. But just the former part would be a good start.

[Sreeram Ramachandran]

Created attachment 96495 [details]
Patch

[Sreeram Ramachandran]

(In reply to comment #3)
> Created an attachment (id=96495) [details]
> Patch

Some notes on the above patch:

1. I've moved the "Preserve Log upon Navigation" button to global scope (so the setting is now shared by the Network and Console panels). There might be an expectation that this will also be honoured for other panels (Timeline? Resources?), but currently, it has no effect on them.

2. In the Timeline panel, there's already a similar looking button, which makes for ugly UI, since now both buttons are visible. I'd like to change the glyph of the preserve-log button to something else, but don't have good ideas (perhaps a padlock icon?). Any UI design help would be appreciated!

3. I've added a new "frameNavigated" message just so that the frontend can determine whether to request the backend to clear messages. Is there a way to query the WebInspector.preserveLog property directly from the backend? If so, we could get rid of this message altogether.

4. I couldn't see a simple way to trigger the frameNavigated message in the inspector/protocol/console-agent layout test, so it's currently "not checked".

5. I haven't added any new test for this setting, mainly because I didn't see an existing test covering the current network-panel-only preserveLog setting. Please advise if I should go ahead and add a test anyway (which should cover the usage of this setting by both the Network and Console panels).

[Pavel Feldman]

> 1. I've moved the "Preserve Log upon Navigation" button to global scope (so the setting is now shared by the Network and Console panels). There might be an expectation that this will also be honoured for other panels (Timeline? Resources?), but currently, it has no effect on them.

This is a bit confusing.

> 2. In the Timeline panel, there's already a similar looking button, which makes for ugly UI, since now both buttons are visible. I'd like to change the glyph of the preserve-log button to something else, but don't have good ideas (perhaps a padlock icon?). Any UI design help would be appreciated!

We don't have a dedicated designer, so you should do your best :)

> 3. I've added a new "frameNavigated" message just so that the frontend can determine whether to request the backend to clear messages. Is there a way to query the WebInspector.preserveLog property directly from the backend? If so, we could get rid of this message altogether.

I am not sure there is a need for this. There is a number of frameNavigated signals in the front-end already. We should always clear backend objects (as per Yury's comment above), but conditionally wipe them out on the front-end side. As a result, pre-navigation objects will be "dead" (non-expandable, not pointing to any page instances), but still visible in the UI.

I'd suggest that you add a checkbox to the console context menu (the one next to the XHR logging toggle) in order not to mess with the UI challenges for now. I don't see anything wrong with the patch code-wise, but I'll r- it until we figure out the UI bit + object lifetime.

[Sreeram Ramachandran]

(In reply to comment #5)
> > 3. I've added a new "frameNavigated" message just so that the frontend can determine whether to request the backend to clear messages. Is there a way to query the WebInspector.preserveLog property directly from the backend? If so, we could get rid of this message altogether.
> 
> I am not sure there is a need for this. There is a number of frameNavigated signals in the front-end already. We should always clear backend objects (as per Yury's comment above), but conditionally wipe them out on the front-end side. As a result, pre-navigation objects will be "dead" (non-expandable, not pointing to any page instances), but still visible in the UI.

I don't know the implications of not clearing the backend objects. Could you explain why it's important to do so? On first glance, it seems to me to be reasonable to keep them around. Could stuff like "m_previousMessage" lead to problems if the backend and frontend have different lists of messages?

To be clear, my current patch doesn't let you expand dead objects either, so it _appears_ to have no functional difference to clearing the backend objects, but then again, I'm totally new to this part of the code, and I don't know the logic involved.

> I'd suggest that you add a checkbox to the console context menu (the one next to the XHR logging toggle) in order not to mess with the UI challenges for now. I don't see anything wrong with the patch code-wise, but I'll r- it until we figure out the UI bit + object lifetime.

I can certainly implement this, but I'd strongly encourage you to consider a more uniform UI like in the current patch. It seems confusing to have two different ways of achieving the same functionality in two panels (especially if one is a button and the other tucked away in a context menu).

If the icon is the only thing that's confusing, I can try my hand at adding something different. If it's also a matter of other panels not respecting the setting (yet), that should give us more impetus to add similar functionality to those panels as soon as possible. I think it would be fantastic to have all panels implement "preserve log" functionality.

[Yury Semikhatsky]

(In reply to comment #6)
> (In reply to comment #5)
> > > 3. I've added a new "frameNavigated" message just so that the frontend can determine whether to request the backend to clear messages. Is there a way to query the WebInspector.preserveLog property directly from the backend? If so, we could get rid of this message altogether.
> > 
> > I am not sure there is a need for this. There is a number of frameNavigated signals in the front-end already. We should always clear backend objects (as per Yury's comment above), but conditionally wipe them out on the front-end side. As a result, pre-navigation objects will be "dead" (non-expandable, not pointing to any page instances), but still visible in the UI.
> 
> I don't know the implications of not clearing the backend objects. Could you explain why it's important to do so? On first glance, it seems to me to be reasonable to keep them around.

Not clearing them may prevent garbage collection of potentially all JavaScript objects in the old document, we don't want such leaks.

> Could stuff like "m_previousMessage" lead to problems if the backend and frontend have different lists of messages?
>
Yes, it could. m_previousMessage message from the old document may match a message in the new document.

I agree with Pavel, we shouldn't try to preserve messages on the backend, keeping them in the frontend will be enough for reading their message text. Also, if some messages are preserved after navigation, we should probably mark navigation point in the console log.

[Pavel Feldman]

> To be clear, my current patch doesn't let you expand dead objects either, so it _appears_ to have no functional difference to clearing the backend objects, but then again, I'm totally new to this part of the code, and I don't know the logic involved.

We do, and we are suggesting that you clear the list!

> I can certainly implement this, but I'd strongly encourage you to consider a more uniform UI like in the current patch. It seems confusing to have two different ways of achieving the same functionality in two panels (especially if one is a button and the other tucked away in a context menu).

Console can be accessed not only as a panel, but as a drawer on any other panel as well (via pressing Esc). Preserving log does make sense both for Network panel and Console, but it does not make sense for the rest of the panels. So putting it everywhere will be confusing, especially on the Timeline, Profiler and Audit panels.

> If the icon is the only thing that's confusing, I can try my hand at adding something different. If it's also a matter of other panels not respecting the setting (yet), that should give us more impetus to add similar functionality to those panels as soon as possible. I think it would be fantastic to have all panels implement "preserve log" functionality.

I disagree here, I don't see how Elements or Resources can possibly "preserve log". The information they expose is state-driven, not log-driven. So no, the icon is not the only concern. My advice for you was that if you want to have this change landed in a foreseeable future, you should minimize the surface of the UI changes. Context menu seems like a good fit.

[Sreeram Ramachandran]

Created attachment 96666 [details]
Moves setting to context menu, clears backend messages

[Sreeram Ramachandran]

(In reply to comment #9)
> Created an attachment (id=96666) [details]
> Moves setting to context menu, clears backend messages

Reviewers, ping?

[Pavel Feldman]

Comment on attachment 96666 [details]
Moves setting to context menu, clears backend messages

View in context: https://bugs.webkit.org/attachment.cgi?id=96666&action=review

A couple of nits, otherwise looks good.

> Source/WebCore/ChangeLog:13
> +        No new tests. (OOPS!)

I think it is possible to test this. Check out LayoutTests/inspector/console/console-log-toString-object.html on how to reload a page and dump console messages afterwards. function test() is executed within inspector context, so you should be able to set your preserve flag there prior to calling the reload.

> Source/WebCore/inspector/front-end/ConsoleView.js:465
> +        contextMenu.appendCheckboxItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Preserve log" : "Preserve Log"), preserveLogItemAction, WebInspector.settings.preserveConsoleLog);

This string should be added to the WebCore/English.lproj/localizedStrings.js

[Sreeram Ramachandran]

Created attachment 97129 [details]
Added message to localizedStrings and a test

[WebKit Review Bot]

Comment on attachment 97129 [details]
Added message to localizedStrings and a test

Clearing flags on attachment: 97129

Committed r88815: <http://trac.webkit.org/changeset/88815>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.