[Webkit-unassigned] [Bug 33096] [Qt] DRT: Support evaluateInWebInspector(), setTimerProfilingEnabled() and display().

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33096





--- Comment #18 from Robert Hogan <robert at roberthogan.net>  2010-01-14 11:17:27 PST ---
(In reply to comment #17)
> (In reply to comment #15)
> > Created an attachment (id=46511)
 --> (https://bugs.webkit.org/attachment.cgi?id=46511) [details] [details]
> > Updated Patch
> > 
> > I think this is an improvement - only call closewebinspector() when necessary.
> > Also clean up some whitespace.
> 
> Cool, looking better!
> I'm not a reviewer but here are some comments:
> - The link to bugzilla should go right below the summary, that way I don't have
> to scan the rest of the changelog for it.
> - Spelling error in changelog (setTimerProfiilingEnabled).
> - qt_drt_webinspector_execute_script --> qt_drt_webinspector_executeScript
> (following the naming convention of other qt_drt functions, e.g.
> qt_drt_overwritePluginDirectories).
> - Rather than doing the URL acrobatics in DumpRenderTree::open(), I recommend
> having a shouldOpenWebInspector() helper function (see Mac and GTK port), much
> more readable.
> 
> - I don't think LayoutController::hideWebInspector() should be removed, it
> should rather be renamed to closeWebInspector() (looks like it was just given
> the wrong name in the original impl).
> - closeWebInspector() should set the DeveloperExtrasEnabled attribute to false
> after calling close(), as per the Mac and GTK implementations.
> - Should closeWebInspector() really check m_webInspector? Since
> showWebInspector() bypasses the QWebInspector API, it doesn't look symmetric.

No problem!

> - In general I'd like to see the inspector-related part of the patch split from
> the rest (display(), setTimelineProfilingEnabled()), if that makes sense.
> Unless they are tightly related.

They are, in the sense that they're required for all twenty tests to pass -
timelineprofiling for about six or seven, display() for just one.


> Looking forward to being able to run those inspector tests!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.