Last Comment Bug 660721 - move nsPluginInstanceOwner to its own files in dom/plugins/base/
: move nsPluginInstanceOwner to its own files in dom/plugins/base/
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Josh Aas
:
Mentors:
Depends on: 662089 670230
Blocks: 90268 598425
  Show dependency treegraph
 
Reported: 2011-05-30 17:29 PDT by Josh Aas
Modified: 2011-07-08 13:30 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (127.90 KB, patch)
2011-05-30 20:52 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.1 (254.35 KB, patch)
2011-05-30 21:18 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.2 (255.58 KB, patch)
2011-05-30 23:32 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.3 (256.53 KB, patch)
2011-05-31 08:13 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.4 (256.56 KB, patch)
2011-05-31 16:01 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.5 (245.81 KB, patch)
2011-05-31 18:51 PDT, Josh Aas
roc: review+
Details | Diff | Review
fix v1.6 (245.88 KB, patch)
2011-05-31 20:03 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.7 (245.95 KB, patch)
2011-05-31 21:36 PDT, Josh Aas
no flags Details | Diff | Review

Description Josh Aas 2011-05-30 17:29:43 PDT
We should move nsPluginInstanceOwner to its own files in dom/plugins/base/. This is part of bug 90268, after which nsObjectFrame will no longer be the instance owner.
Comment 1 Josh Aas 2011-05-30 20:52:43 PDT
Created attachment 536220 [details] [diff] [review]
fix v1.0
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-30 21:03:35 PDT
You forgot to attach the new files.

You might get a nice diff if you hg copy nsObjectFrame.cpp to nsPluginInstanceOwner.cpp and then modify the latter.
Comment 3 Josh Aas 2011-05-30 21:18:07 PDT
Created attachment 536222 [details] [diff] [review]
fix v1.1

Add new files. I've only compiled and run this on Mac OS X so far. Working on Linux now.
Comment 4 Josh Aas 2011-05-30 23:32:37 PDT
Created attachment 536239 [details] [diff] [review]
fix v1.2

Linux build fixes.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London) 2011-05-31 01:05:06 PDT
Comment on attachment 536239 [details] [diff] [review]
fix v1.2

>+  PRInt32 blockPopups =
>+  Preferences::GetInt("privacy.popups.disable_from_plugins");

nit: fix the wrong indentation.
Comment 6 Josh Aas 2011-05-31 08:13:16 PDT
Created attachment 536307 [details] [diff] [review]
fix v1.3
Comment 7 Josh Aas 2011-05-31 13:07:07 PDT
This patch doesn't minimize the number of #includes in nsObjectFrame.cpp but we can take care of that in followup. This should be good enough for round one.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-31 15:58:20 PDT
This patch doesn't do what I suggested in comment #2 ... any particular reason why?
Comment 9 Josh Aas 2011-05-31 16:01:21 PDT
Created attachment 536459 [details] [diff] [review]
fix v1.4

QT build fixes.
Comment 10 Josh Aas 2011-05-31 16:02:10 PDT
No reason, I just had it started a different way. I can do that if you want.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-31 17:40:21 PDT
You should just be able to move aside your new file, "hg copy" nsObjectFrame.cpp over, then replace the copy with your new file. If we're lucky we'll get a nicer diff.
Comment 12 Josh Aas 2011-05-31 18:51:02 PDT
Created attachment 536501 [details] [diff] [review]
fix v1.5

This is a patch produced by moving nsObjectFrame.cpp.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-31 19:03:40 PDT
Comment on attachment 536501 [details] [diff] [review]
fix v1.5

Review of attachment 536501 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that fixed

::: dom/plugins/base/Makefile.in
@@ +128,5 @@
>  endif
>  
>  LOCAL_INCLUDES = \
>    -I$(topsrcdir)/xpcom/base/ \
> +  $(MOZ_CAIRO_CFLAGS) \

This doesn't seem right, LOCAL_INCLUDES should just include -I options, right?

I know you just copied this, but why not add them to CXXFLAGS instead?
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-31 19:04:05 PDT
Comment on attachment 536501 [details] [diff] [review]
fix v1.5

Review of attachment 536501 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 15 Josh Aas 2011-05-31 19:21:04 PDT
I had originally put a bunch of the functions in a different order, the diff wasn't very nice until I put them back in the correct order. I did a bunch of that, not all of it though. Thanks for the review!
Comment 16 Josh Aas 2011-05-31 19:21:29 PDT
s/correct order/original order/
Comment 17 Josh Aas 2011-05-31 20:03:11 PDT
Created attachment 536508 [details] [diff] [review]
fix v1.6

Makefile change that roc suggested.
Comment 18 Josh Aas 2011-05-31 21:36:41 PDT
Created attachment 536522 [details] [diff] [review]
fix v1.7

Another QT build fix.
Comment 19 Josh Aas 2011-05-31 21:39:49 PDT
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/c4999efc5a84
Comment 20 Josh Aas 2011-05-31 22:29:36 PDT
bustage fix

http://hg.mozilla.org/mozilla-central/rev/e63c5dba5f4d

Note You need to log in before you can comment on or make changes to this bug.