Bug 56678 - Add API - QWebUserEnvironment
Summary: Add API - QWebUserEnvironment
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 56274 56482
Blocks: 41801 50895
  Show dependency treegraph
 
Reported: 2011-03-18 14:07 PDT by Robert Hogan
Modified: 2012-12-26 07:09 PST (History)
10 users (show)

See Also:


Attachments
Patch (22.98 KB, patch)
2011-03-18 14:10 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (23.39 KB, patch)
2011-03-18 14:16 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (21.39 KB, patch)
2011-03-18 15:01 PDT, Robert Hogan
vestbo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2011-03-18 14:07:23 PDT
Add API - QWebUserEnvironment
Comment 1 Robert Hogan 2011-03-18 14:10:03 PDT
Created attachment 86207 [details]
Patch
Comment 2 Robert Hogan 2011-03-18 14:14:09 PDT
Still quite raw obviously. It depends on the patches in 56274 and 56482. Ultimately CSS font detection will need to be delegated to this class (or something like it) as well.

See https://trac.webkit.org/wiki/Fingerprinting for what I'm trying to address here.
Comment 3 Robert Hogan 2011-03-18 14:16:26 PDT
Created attachment 86210 [details]
Patch
Comment 4 Robert Hogan 2011-03-18 14:33:43 PDT
At first glance this class seems like a bit of a rattle-bag, and I initially intended to find a home in QWebFrame, QWebPage and co. for each of the properties delegated here. But in the end it seemed more developer-friendly to put everything in one place. 

The documentation in the class needs to be clearer that it is not a silver bullet and there needs to be more of the Fingerprinting wiki page in there. I'm also not totally comfortable with leaving Screen and Navigator entirely to QWebFrame::addJavascriptToWindowObject(). It might be better to put all privacy-relevant properties in one place. Also, the class needs a better name.

So I would like feedback on what I'm trying to do, not only here but on 56274 and 56482, which this depends on.
Comment 5 Robert Hogan 2011-03-18 15:01:30 PDT
Created attachment 86218 [details]
Patch
Comment 6 WebKit Review Bot 2011-03-18 15:04:58 PDT
Attachment 86218 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/headers.pri', u'Sourc..." exit_code: 1

Source/WebKit/qt/Api/qwebuserenvironment.h:21:  #ifndef header guard has wrong style, please use: WTF_qwebuserenvironment_h  [build/header_guard] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Benjamin Poulain 2011-03-18 16:50:39 PDT
I strongly advice starting a discussion on the mailing list to introduce any important change of API.

APIs are our tools to provide features, but it is at the same time our biggest curse because the smallest error tend to cost us a lot in maintenance, support, and can prevent us from future improvement.

Contributors who do not deal with the downside of mistake in APIs are generally not aware of that.

In this case, I understand what you are trying to do. I want to help you do that since you are doing a good job on the webkit project. But at the same time, I do not think this API is even close to something that can be integrated in a released version of QtWebKit.

First, I think you should explore the possibility of a minimal private API for your project. Since you work both on WebKit.org, and the browser, you have the full flexibility to change the browser to match the implementation.

(Please also note that the timing is pretty bad since we are stabilizing WebKit trunk with the objective of forking for QtWebKit 2.2. The stabilization will surely give less visibility to this particular work)
Comment 8 Robert Hogan 2011-03-19 04:54:19 PDT
(In reply to comment #7)
> 
> First, I think you should explore the possibility of a minimal private API for your project. Since you work both on WebKit.org, and the browser, you have the full flexibility to change the browser to match the implementation.

That's a really good idea and particularly useful here as the best way of exposing the information browsers need to deal with fingerprinting is likely to develop over time. A private API in DumpRenderTreeSupportQt or similar is exactly the sort of place to get a better handle on the final API requirements.

Do you have any concerns about the changes in 56274 and 56482?
Comment 9 Kenneth Rohde Christiansen 2011-03-19 05:15:34 PDT
(In reply to comment #7)
> I strongly advice starting a discussion on the mailing list to introduce any important change of API.
> 
> APIs are our tools to provide features, but it is at the same time our biggest curse because the smallest error tend to cost us a lot in maintenance, support, and can prevent us from future improvement.
> 
> Contributors who do not deal with the downside of mistake in APIs are generally not aware of that.
> 
> In this case, I understand what you are trying to do. I want to help you do that since you are doing a good job on the webkit project. But at the same time, I do not think this API is even close to something that can be integrated in a released version of QtWebKit.
> 
> First, I think you should explore the possibility of a minimal private API for your project. Since you work both on WebKit.org, and the browser, you have the full flexibility to change the browser to match the implementation.
> 
> (Please also note that the timing is pretty bad since we are stabilizing WebKit trunk with the objective of forking for QtWebKit 2.2. The stabilization will surely give less visibility to this particular work)

The API basically exposes the Chrome Client. Also changing most of these will result in wrongly shown contents (especially on mobile platforms), so if not treated the right way it can be a can of wormes. We need to be very careful about this.
Comment 10 Robert Hogan 2011-03-19 05:53:34 PDT
(In reply to comment #9)
> 
> The API basically exposes the Chrome Client. Also changing most of these will result in wrongly shown contents (especially on mobile platforms), so if not treated the right way it can be a can of wormes. We need to be very careful about this.

You're right. The pros and cons the client needs to weigh up are outlined here:

https://trac.webkit.org/wiki/Fingerprinting#WindowObject

and in the links there. The approach of Torbutton and Torora is to restrict the size of the actual window, e.g. by multiples of px, so that the rendered content is not horribly disconnected from the actual window size. Another policy a client could use is to fix on a minimum rendered page size and use that regardless if the window is actually larger.

This is not a very pleasant trade-off to have to make and the right choice will differ from platform to platform and client to client.
Comment 11 Tor Arne Vestbø 2011-04-26 16:55:56 PDT
Comment on attachment 86218 [details]
Patch

Agree with Benjamin, a bit more discussion is needed.