Bug 84559 – [Blackberry] remove m_isRequestedByPlugin in ResourceRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Chris.Guan]

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.

[Chris.Guan]

Created attachment 138288 [details]
Patch

[Antonio Gomes]

+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..

[Chris.Guan]

(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.

[Chris.Guan]

(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;

[Chris.Guan]

(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

[Antonio Gomes]

please post a patch :)

[Chris.Guan]

Created attachment 140690 [details]
Patch

[Chris.Guan]

(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.

[Gyuyoung Kim]

Comment on attachment 140690 [details]
Patch

Attachment 140690 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12650304

[Chris.Guan]

(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.

[Antonio Gomes]

(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.

[WebKit Review Bot]

Comment on attachment 140690 [details]
Patch

Clearing flags on attachment: 140690

Committed r116418: <http://trac.webkit.org/changeset/116418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.