Bug 80164 - [Qt] QWebNavigationRequest 'action' property should have an enum type instead of int
Summary: [Qt] QWebNavigationRequest 'action' property should have an enum type instead...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks: 78821
  Show dependency treegraph
 
Reported: 2012-03-02 08:56 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-03-07 04:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.74 KB, patch)
2012-03-02 09:03 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (5.88 KB, patch)
2012-03-05 11:40 PST, Caio Marcelo de Oliveira Filho
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2012-03-02 08:56:02 PST
[Qt] QWebNavigationRequest 'action' property should have an enum type instead of int
Comment 1 Caio Marcelo de Oliveira Filho 2012-03-02 09:03:31 PST
Created attachment 129909 [details]
Patch
Comment 2 Rafael Brandao 2012-03-05 09:43:49 PST
Comment on attachment 129909 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86
> +        RequestActionCount,

Shouldn't this be named ActionCountRequest instead (to follow other names pattern)?
Comment 3 Caio Marcelo de Oliveira Filho 2012-03-05 09:49:15 PST
Comment on attachment 129909 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86
>> +        RequestActionCount,
> 
> Shouldn't this be named ActionCountRequest instead (to follow other names pattern)?

It is the Count of RequestActions, it is not an action to be performed itself.
Comment 4 Tor Arne Vestbø 2012-03-05 09:55:27 PST
Comment on attachment 129909 [details]
Patch

after discussion on IRC
Comment 5 Caio Marcelo de Oliveira Filho 2012-03-05 11:40:13 PST
Created attachment 130167 [details]
Patch
Comment 6 Simon Hausmann 2012-03-06 10:09:05 PST
Comment on attachment 130167 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86
> +        IgnoreRequest = 0xFF

Why not simply leave it as its original value and use DownloadRequest = IgnoreRequest + 1?
It seems strange to have a gap in the _existing_ enums.

or

   enum NavigationRequestAction {
       AcceptRequest,
       IgnoreRequest,
       LastRequestAction = IgnoreRequest /* Mark internal in docs! */
   }

    DownloadRequest = LastRequest

I recall seeing the pattern like this in Qt. (see qnamespace.h)
Comment 7 Caio Marcelo de Oliveira Filho 2012-03-06 13:32:58 PST
Comment on attachment 130167 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86
>> +        IgnoreRequest = 0xFF
> 
> Why not simply leave it as its original value and use DownloadRequest = IgnoreRequest + 1?
> It seems strange to have a gap in the _existing_ enums.
> 
> or
> 
>    enum NavigationRequestAction {
>        AcceptRequest,
>        IgnoreRequest,
>        LastRequestAction = IgnoreRequest /* Mark internal in docs! */
>    }
> 
>     DownloadRequest = LastRequest
> 
> I recall seeing the pattern like this in Qt. (see qnamespace.h)

In Qt we use this pattern for ItemDataRole, people create their roles with values >= UserRole. The difference is that functions taking roles as parameter actually take int (see QAbstractItemModel methods). 

If we want to use the enum type as the property type and as storage for the property (member m_action is of the enum type), we are saying we accept only values inside enum range. It may work in GCC if the value is out of range, but GCC still is unhappy when we construct switch() statements using the type. Casting to int in the switch() alleviates this second problem, but with no true guarantee.

Tor-Arne explicitly preferred we do not "clutter" the QML API with the "LastRequestAction" or "RequestActionCount".
Comment 8 Caio Marcelo de Oliveira Filho 2012-03-07 04:29:21 PST
Committed r110047: <http://trac.webkit.org/changeset/110047>