1038570 - Rename nsObjectFrame to nsPluginFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

It is confusing that <object> doesn't always produce nsObjectFrame but only plugin <object>s.

Rename nsObjectFrame to nsPluginFrame to reflect this.

Comment 1

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

This probably should have been done as a followup to bug 1156
https://github.com/mozilla/gecko-dev/commit/7002686d0d89718f9cb5531319ab5da97e68a474
as I suggested in bug 1156 comment 4.

Comment 2

[Jet Villegas (inactive)]

Pedro: take a crack at this one?

Comment 3

[kartik.rao24]

Attachment: Patch with the changes for renaming nsObjectFrame to nsPluginFrame

Can you please let me know if you are planning to work on this bug? If not, i have modified the files and have uploaded the patch.

Comment 4

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

The diff to nsObjectFrame.cpp/nsPluginFrame.cpp and .h appears to be missing from your patch.  (If you need help with Mercurial, let somebody know in the bug, or ask on #mercurial or #developers on irc.mozilla.org.)

Comment 5

[kartik.rao24]

I have deleted the nsObjectFrame.h/cpp files and have added the nsPluginFrame.h/cpp files. Even addition/deletion changes should be visible on patch?

Comment 6

[kartik.rao24]

Attachment: Patch for add/delete of .h/cpp files

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

The addition still isn't visible.

You should also record the change as a move in hg (using "hg mv --after", see "hg help mv") so that the history and blame for the file follows it.

It would also be better to repost the corrected patch as a single patch rather than as two separate patches.

Comment 8

[Saurabh Anand [:sawrubh]]

Attachment: Patch (v1)

Comment 9

[kartik.rao24]

Attachment: Complete Patch

Patch with all the changes

Comment 10

[rijulhanda]

Attachment: Patch (v1)

This invalidates sawrubh's patch. He had uploaded my patch by mistake.

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Comment on attachment 8477954 [details] [diff] [review]
Complete Patch

This patch still doesn't have the rename of the files recorded as a move; it shows here as a removal and an addition, which is wrong, and which will make the patch hard to review and unacceptable to land.


That said, otherwise, this patch looks more complete than the other patch, since this patch renames the file and renames a bunch of variables that should be renamed.  Though those steps could perhaps have been better done in a series of stacked patches rather than one combined patch.

It's not clear to me why three of you decided to work on this on the same day, though.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

I'm not sure who wants to take the last stab on this but please use |hg mv| to rename files (comment 7) and make rename files and rename variables two or more stacked patches (comment 11) as advised by :dbaron.

Comment 13

[Arnaud Sourioux [:Six]]

Attachment: nsobject_to_nsplugin_move_files.patch Part 1/3

This patch only moves the files with 'hg mv'

Comment 14

[Arnaud Sourioux [:Six]]

Attachment: nsobject_to_nsplugin_rename_var_in_cpp_hpp_h.patch Part 2/3

This patch renames classes and variables in C++ files (.h .hpp .cpp)

Comment 15

[Arnaud Sourioux [:Six]]

Attachment: nsobject_to_nsplugin_rename_var_in_other_files.patch Part 3/3

This patch renames variables in non C++ files (.mm anf moz.build)

Comment 16

[Arnaud Sourioux [:Six]]

Tbpl is green with thoses 3 stacked patch applied:
https://tbpl.mozilla.org/?tree=Try&rev=c86c71441332

Comment 17

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Comment on attachment 8483438 [details] [diff] [review]
nsobject_to_nsplugin_move_files.patch Part 1/3

This isn't a useful set of stacked patches since they don't compile on their own.  You should squash them together when landing (but please don't lose the rename data).

What I was suggesting was one patch that renames the files, which would also change the moz.build files and #includes elsewhere, and then another patch to rename the class, and perhaps a third patch to rename variables (such as variables called "objectFrame") to match the rest of the renaming.

Note that attachment 8477954 [details] [diff] [review] contained that third patch, whereas this one doesn't.


r=dbaron anyway, conditional on squashing the patches

Comment 18

[Arnaud Sourioux [:Six]]

Attachment: nsobject_to_nsplugin_move_files_rename_includes_mozbuild.patch Part 1/3

Keeping previous dbaron r+

Comment 19

[Arnaud Sourioux [:Six]]

Attachment: nsobject_to_nsplugin_rename_classes.patch Part 2/3

Keeping previous dbaron r+

Comment 20

[Arnaud Sourioux [:Six]]

Attachment: nsobject_to_nsplugin_rename_variables.patch Part 3/3

Keeping previous dbaron r+

Comment 21

[Arnaud Sourioux [:Six]]

Thoses patches are taking care of dbaron's last comment.
Sorry for the misundertanding, i'm not familiar with stacked patchs but now it should be better.
Just let me know if it's what you expect.

Thanks,

Comment 22

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Yes, this looks great.  Thanks.

(It would have been ok to just land as 1 big patch.  This is better, though it's not obvious that it's worth the work to fix it up after having done it once already.)

Comment 23

[Arnaud Sourioux [:Six]]

https://tbpl.mozilla.org/?tree=Try&rev=a6c3c48590fc
TBPL build is green

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22deb0f2260d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de156cbeddd
https://hg.mozilla.org/integration/mozilla-inbound/rev/03ed854c4872

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22deb0f2260d
https://hg.mozilla.org/mozilla-central/rev/1de156cbeddd
https://hg.mozilla.org/mozilla-central/rev/03ed854c4872