660721 - move nsPluginInstanceOwner to its own files in dom/plugins/base/

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

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]

Attachment: fix v1.0

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]

Attachment: 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]

Attachment: fix v1.2

Linux build fixes.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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]

Attachment: fix v1.3

Comment 7

[Josh Aas]

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) (email my personal email if necessary)]

This patch doesn't do what I suggested in comment #2 ... any particular reason why?

Comment 9

[Josh Aas]

Attachment: fix v1.4

QT build fixes.

Comment 10

[Josh Aas]

No reason, I just had it started a different way. I can do that if you want.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]

Attachment: fix v1.5

This is a patch produced by moving nsObjectFrame.cpp.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Comment on attachment 536501 [details] [diff] [review]
fix v1.5

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

Comment 15

[Josh Aas]

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]

s/correct order/original order/

Comment 17

[Josh Aas]

Attachment: fix v1.6

Makefile change that roc suggested.

Comment 18

[Josh Aas]

Attachment: fix v1.7

Another QT build fix.

Comment 19

[Josh Aas]

pushed to mozilla-central

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

Comment 20

[Josh Aas]

bustage fix

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