Bug 84559 - [Blackberry] remove m_isRequestedByPlugin in ResourceRequest
Summary: [Blackberry] remove m_isRequestedByPlugin in ResourceRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-22 20:32 PDT by Chris.Guan
Modified: 2012-05-08 07:01 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.75 KB, patch)
2012-04-22 20:46 PDT, Chris.Guan
no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2012-05-08 01:29 PDT, Chris.Guan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris.Guan 2012-04-22 20:32:52 PDT
As AP and Antonio suggestions, m_isRequestedByPlugin should not be in ResourceRequest which is a network level abstraction and it does not fix any issues any more. So clean up all related code for m_isRequestedByPlugin.
Comment 1 Chris.Guan 2012-04-22 20:46:31 PDT
Created attachment 138288 [details]
Patch
Comment 2 Antonio Gomes 2012-04-23 07:15:11 PDT
+gen.

As I remember, it was added to avoid popping up a window when start scrolling a page off of a Flash Ad (for example, cnn.com full edition, right side of the webpage). However it did not completely cover all cases, and popups are still opened in many real world web sites ever with this hack.

As I said, it does fixes some cases, but not all..
Comment 3 Chris.Guan 2012-04-23 19:39:15 PDT
(In reply to comment #2)
> +gen.
> 
> As I remember, it was added to avoid popping up a window when start scrolling a page off of a Flash Ad (for example, cnn.com full edition, right side of the webpage). However it did not completely cover all cases, and popups are still opened in many real world web sites ever with this hack.
> 
> As I said, it does fixes some cases, but not all..

I was searching our code, only dispatchDecidePolicyForNewWindowAction is using "isRequestedByPlugin", do we have other cases?
I removed the code in dispatchDecidePolicyForNewWindowAction, because isRequestedByPlugin seems to be always false without my that fixing(https://bugs.webkit.org/show_bug.cgi?id=83447), and the code run well long time without hitting those code I removed.
Comment 4 Chris.Guan 2012-04-24 03:57:35 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > +gen.
> > 
> > As I remember, it was added to avoid popping up a window when start scrolling a page off of a Flash Ad (for example, cnn.com full edition, right side of the webpage). However it did not completely cover all cases, and popups are still opened in many real world web sites ever with this hack.
> > 
> > As I said, it does fixes some cases, but not all..
> 
> I was searching our code, only dispatchDecidePolicyForNewWindowAction is using "isRequestedByPlugin", do we have other cases?
> I removed the code in dispatchDecidePolicyForNewWindowAction, because isRequestedByPlugin seems to be always false without my that fixing(https://bugs.webkit.org/show_bug.cgi?id=83447), and the code run well long time without hitting those code I removed.

Gen & Antonio,
I guess the first policyChecker is useless, because there is not an early return. So the second policyChecker is working actually. let me summarize the options I have:
1. remove them, just like my patch 
2. just remove "request.isRequestedByPlugin()"
3. #2 + early return;
Comment 5 Chris.Guan 2012-04-25 03:44:49 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > +gen.
> > > 
> > > As I remember, it was added to avoid popping up a window when start scrolling a page off of a Flash Ad (for example, cnn.com full edition, right side of the webpage). However it did not completely cover all cases, and popups are still opened in many real world web sites ever with this hack.
> > > 
> > > As I said, it does fixes some cases, but not all..
> > 
> > I was searching our code, only dispatchDecidePolicyForNewWindowAction is using "isRequestedByPlugin", do we have other cases?
> > I removed the code in dispatchDecidePolicyForNewWindowAction, because isRequestedByPlugin seems to be always false without my that fixing(https://bugs.webkit.org/show_bug.cgi?id=83447), and the code run well long time without hitting those code I removed.
> 
> Gen & Antonio,
> I guess the first policyChecker is useless, because there is not an early return. So the second policyChecker is working actually. let me summarize the options I have:
> 1. remove them, just like my patch 
> 2. just remove "request.isRequestedByPlugin()"
> 3. #2 + early return;

Gen & Antonio,
    It seems that it is working well for option#3 by my testing(cnn.com). If you guys agree with me, I could post a new patch with my option#3, or do you have a better solution? Thanks
Comment 6 Antonio Gomes 2012-04-25 04:54:25 PDT
please post a patch :)
Comment 7 Chris.Guan 2012-05-08 01:29:32 PDT
Created attachment 140690 [details]
Patch
Comment 8 Chris.Guan 2012-05-08 01:35:21 PDT
(In reply to comment #7)
> Created an attachment (id=140690) [details]
> Patch

With the latest refactor for zoom/scroll from Gen, it works well. I guess Gen's refactor make plugin happy by removing m_isRequestedByPlugin.
Comment 9 Gyuyoung Kim 2012-05-08 02:12:12 PDT
Comment on attachment 140690 [details]
Patch

Attachment 140690 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12650304
Comment 10 Chris.Guan 2012-05-08 02:39:44 PDT
(In reply to comment #9)
> (From update of attachment 140690 [details])
> Attachment 140690 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/12650304

false alarm? I did not touch any make files and elf code.
Comment 11 Antonio Gomes 2012-05-08 06:54:44 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=140690) [details] [details]
> > Patch
> 
> With the latest refactor for zoom/scroll from Gen, it works well. I guess Gen's refactor make plugin happy by removing m_isRequestedByPlugin.

not sure how it is related, in fact.
Comment 12 WebKit Review Bot 2012-05-08 07:01:50 PDT
Comment on attachment 140690 [details]
Patch

Clearing flags on attachment: 140690

Committed r116418: <http://trac.webkit.org/changeset/116418>
Comment 13 WebKit Review Bot 2012-05-08 07:01:58 PDT
All reviewed patches have been landed.  Closing bug.