Bug 100224 - [Qt] Detach WebContext's clients from QtWebContext
Summary: [Qt] Detach WebContext's clients from QtWebContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks: 70236 76773
  Show dependency treegraph
 
Reported: 2012-10-24 03:46 PDT by Jocelyn Turcotte
Modified: 2012-10-25 04:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.25 KB, patch)
2012-10-24 04:41 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-10-24 03:46:22 PDT
[Qt] Detach WebContext's clients from QtWebContext
Comment 1 Jocelyn Turcotte 2012-10-24 04:41:20 PDT
Created attachment 170363 [details]
Patch
Comment 2 Jocelyn Turcotte 2012-10-24 07:01:20 PDT
*** Bug 95968 has been marked as a duplicate of this bug. ***
Comment 3 Csaba Osztrogonác 2012-10-24 07:05:49 PDT
I don't understand why we need new bug report for it ...
At least I had to add the dependency again :)
Comment 4 Simon Hausmann 2012-10-24 11:37:42 PDT
Comment on attachment 170363 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:53
> +    url.setPath(QLatin1Char('/') + QString::number(WTF::StringHash::hash(iconURL)));

I have the guts feeling that we might change the format of this in the future, perhaps re-introduce a context id?

This makes me wonder if we should introduce a version in the URL, like versions in REST APIs.
Comment 5 Jocelyn Turcotte 2012-10-25 03:44:56 PDT
(In reply to comment #4)
> (From update of attachment 170363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170363&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:53
> > +    url.setPath(QLatin1Char('/') + QString::number(WTF::StringHash::hash(iconURL)));
> 
> I have the guts feeling that we might change the format of this in the future, perhaps re-introduce a context id?
> 
> This makes me wonder if we should introduce a version in the URL, like versions in REST APIs.

If possible it would be nice to prevent/discourage storing the image: URLs and and let them re-query them using http URLs through QWebIconImageProvider::iconURLForPageURLInContext.
Especially if they contain temporary IDs.
Comment 6 Csaba Osztrogonác 2012-10-25 03:47:33 PDT
CQ won't work if the patch isn't applialble ...
Comment 7 Jocelyn Turcotte 2012-10-25 03:54:40 PDT
(In reply to comment #6)
> CQ won't work if the patch isn't applialble ...

It should be appliable since its dependent patch just landed now, hopefully no other conflict has been introduced meanwhile.
Comment 8 Jocelyn Turcotte 2012-10-25 03:56:33 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > CQ won't work if the patch isn't applialble ...
> 
> It should be appliable since its dependent patch just landed now, hopefully no other conflict has been introduced meanwhile.

Eww ok it still doesn't work.
Comment 9 Csaba Osztrogonác 2012-10-25 03:58:37 PDT
It would be great if you can add the dependcy to the depends on list of the bug.
(And next time add r? after the dependencies landed to let the EWS bots test the patch.)
Comment 10 WebKit Review Bot 2012-10-25 04:16:50 PDT
Comment on attachment 170363 [details]
Patch

Clearing flags on attachment: 170363

Committed r132467: <http://trac.webkit.org/changeset/132467>
Comment 11 WebKit Review Bot 2012-10-25 04:16:54 PDT
All reviewed patches have been landed.  Closing bug.