Bug 38867 - [Qt] Unskip fast/loader/main-document-url-for-non-http-loads.htm
Summary: [Qt] Unskip fast/loader/main-document-url-for-non-http-loads.htm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-05-10 14:29 PDT by Robert Hogan
Modified: 2010-06-17 01:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.23 KB, patch)
2010-05-10 14:43 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (21.68 KB, patch)
2010-05-16 11:30 PDT, Robert Hogan
kenneth: review+
Details | Formatted Diff | Diff
remove some unused symbols (1.90 KB, patch)
2010-05-18 16:09 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-05-10 14:29:43 PDT
Fix FrameLoadeClientQt.cpp to print relative filename paths correctly.
Comment 1 Robert Hogan 2010-05-10 14:43:55 PDT
Created attachment 55606 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2010-05-13 11:20:39 PDT
Comment on attachment 55606 [details]
Patch

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:100
 +  void QWEBKIT_EXPORT qt_dump_resource_load_callbacks_path(const QString& path)
That is not how we name private methods, plus shouldn't this be in the DRTSupport class you created?

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:102
 +      dumpResourceLoadCallbacksPath = path + "/";
Is this right on all platforms? Maybe add a comment
Comment 3 Robert Hogan 2010-05-13 13:21:27 PDT
(In reply to comment #2)
> (From update of attachment 55606 [details])
> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:100
>  +  void QWEBKIT_EXPORT qt_dump_resource_load_callbacks_path(const QString& path)
> That is not how we name private methods, plus shouldn't this be in the DRTSupport class you created?

It's in the style of qt_dump_resource_load_callbacks() so let me know what it should be and I'll rename all of them as appropriate.

FrameLoaderClientQt doesn't use the DRTSupport class at all, maybe it should. I'll look into it.
> 
> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:102
>  +      dumpResourceLoadCallbacksPath = path + "/";
> Is this right on all platforms? Maybe add a comment

Good point, will fix/comment.
Comment 4 Kenneth Rohde Christiansen 2010-05-13 13:52:11 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 55606 [details] [details])
> > WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:100
> >  +  void QWEBKIT_EXPORT qt_dump_resource_load_callbacks_path(const QString& path)
> > That is not how we name private methods, plus shouldn't this be in the DRTSupport class you created?
> 
> It's in the style of qt_dump_resource_load_callbacks() so let me know what it should be and I'll rename all of them as appropriate.
> 
> FrameLoaderClientQt doesn't use the DRTSupport class at all, maybe it should. I'll look into it.

qt_dumpResourceLoadCallbacksPath(...)

But isn't this for DRT usage only?
Comment 5 Robert Hogan 2010-05-13 14:56:12 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 55606 [details] [details] [details])
> > > WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:100
> > >  +  void QWEBKIT_EXPORT qt_dump_resource_load_callbacks_path(const QString& path)
> > > That is not how we name private methods, plus shouldn't this be in the DRTSupport class you created?
> > 
> > It's in the style of qt_dump_resource_load_callbacks() so let me know what it should be and I'll rename all of them as appropriate.
> > 
> > FrameLoaderClientQt doesn't use the DRTSupport class at all, maybe it should. I'll look into it.
> 
> qt_dumpResourceLoadCallbacksPath(...)
> 
> But isn't this for DRT usage only?

Yes it is. All the resource callbacks in frameloaderclientqt sit in the DRT on other ports. I didn't move them into DRTSupport because I encountered a hitch I didn't know how to get around and can't remember what it was now. I'll try it again and if it's possible move them all over with this patch.
Comment 6 Robert Hogan 2010-05-16 11:30:02 PDT
Created attachment 56190 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2010-05-16 12:21:08 PDT
Comment on attachment 56190 [details]
Patch

I would probably have suggested you to do this as two separate patches, but just remember that next time.

Great change though :-) Thanks for doing this!

Before you commit, please talk to Laszlo to see if someone depends on these functions that you removed, though I guess not.
Comment 8 Robert Hogan 2010-05-16 12:41:38 PDT
(In reply to comment #7)
> (From update of attachment 56190 [details])
> I would probably have suggested you to do this as two separate patches, but just remember that next time.
> 
> Great change though :-) Thanks for doing this!
> 
> Before you commit, please talk to Laszlo to see if someone depends on these functions that you removed, though I guess not.

I've retained the old style calls at the end of DumpRenderTreeSupportQt so it shouldn't discombobulate(!) anyone using the 'private' API in their clients.
Comment 9 Laszlo Gombos 2010-05-18 16:09:16 PDT
Created attachment 56419 [details]
remove some unused symbols

There was no QtWebKit release made with these symbols, it should be safe to remove them.
Comment 10 WebKit Commit Bot 2010-05-20 08:01:10 PDT
Comment on attachment 56419 [details]
remove some unused symbols

Clearing flags on attachment: 56419

Committed r59836: <http://trac.webkit.org/changeset/59836>
Comment 11 Shinichiro Hamaji 2010-06-17 01:27:46 PDT
I think this issue was fixed by http://trac.webkit.org/changeset/59580

Closing...