762182 - nsScriptLoader.cpp:398:12: warning: unused variable 'rootElement' [-Wunused-variable]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

{
../../../../mozilla/content/base/src/nsScriptLoader.cpp: In member function 'bool nsScriptLoader::ProcessScriptElement(nsIScriptElement*)':
../../../../mozilla/content/base/src/nsScriptLoader.cpp:398:12: warning: unused variable 'rootElement' [-Wunused-variable]
}

Variable was added (& used) here:
https://hg.mozilla.org/mozilla-central/diff/bc10dcdc3b1e/content/base/src/nsScriptLoader.cpp#l1.55

and then its only use was removed in bug 738380, here:
https://hg.mozilla.org/mozilla-central/diff/3eee3ceb400b/content/base/src/nsScriptLoader.cpp#l1.12
leaving us with the build warning.

So it looks like the variable can just be removed.

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

...though the comment saying that we should rely on |rootElement| is still there.

So bug 738380 left that chunk of code with the comment & code disagreeing. Currently it looks like this:

> 395   // Default script language is whatever the root element specifies
> 396   // (which may come from a header or http-meta tag), or if there
> 397   // is no root element, from the script global object.
> 398   Element* rootElement = mDocument->GetRootElement();
> 399   PRUint32 typeID = nsIProgrammingLanguage::JAVASCRIPT;
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#395

Assuming the code is correct & rootElement is safe to remove, then we presumably want to remove that comment, too.

Comment 2

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 630678 [details] [diff] [review]
Patch

Based on bug 738380 comment 0, it sounds like the comment is obsolete & this patch is fine -- but Ms2ger or bz would know for sure.

Kicking review over to Ms2ger.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 630678 [details] [diff] [review]
Patch

Yep, this is correct.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/907796393e09

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/907796393e09