Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*>

RESOLVED FIXED in Firefox 56

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: hsivonen, Assigned: emk)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*> and change GetDocumentCharacterSet() and SetDocumentCharacterSet() accordingly.
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(Reporter)

Comment 2

a year ago
mozreview-review
Comment on attachment 8881094 [details]
Bug 1373984 - Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*>.

https://reviewboard.mozilla.org/r/152408/#review157480

::: dom/base/nsGlobalWindow.cpp:13401
(Diff revision 1)
>    //
>    // Note the algorithm to get the base URI should match the one
>    // used to actually kick off the load in nsWindowWatcher.cpp.
>    nsCOMPtr<nsIDocument> doc = sourceWindow->GetDoc();
>    nsIURI* baseURI = nullptr;
>    nsAutoCString charset(NS_LITERAL_CSTRING("UTF-8")); // default to utf-8

No need to use a string here anymore, AFAICT.

::: dom/html/nsHTMLDocument.cpp:311
(Diff revision 1)
>  
> -      if(requestCharsetSource <= aCharsetSource)
> +      if (requestCharsetSource <= aCharsetSource)
>          return;
>  
> -      if(NS_SUCCEEDED(rv) && IsAsciiCompatible(requestCharset)) {
> +      if (NS_SUCCEEDED(rv)) {
> +        auto encoding = Encoding::ForLabelNoReplacement(requestCharset);

Why not `ForName`?

::: dom/html/nsHTMLDocument.cpp:349
(Diff revision 1)
>    }
>  
>    if(NS_SUCCEEDED(rv) &&
>       !forceCharsetFromDocShell.IsEmpty() &&
>       IsAsciiCompatible(forceCharsetFromDocShell)) {
> -    aCharset = forceCharsetFromDocShell;
> +    auto encoding = Encoding::ForLabelNoReplacement(forceCharsetFromDocShell);

Hasn't this already gone through label resolution on the doc shell level?

::: editor/libeditor/EditorBase.cpp:1195
(Diff revision 1)
>  {
>    nsCOMPtr<nsIDocument> document = GetDocument();
>    if (NS_WARN_IF(!document)) {
>      return NS_ERROR_UNEXPECTED;
>    }
> -  document->SetDocumentCharacterSet(characterSet);
> +  auto encoding = Encoding::ForLabelNoReplacement(characterSet);

Why not `ForName`?

::: parser/html/nsHtml5MetaScanner.cpp:80
(Diff revision 1)
>    , strBuf(jArray<char16_t, int32_t>::newJArray(36))
>    , content(nullptr)
>    , charset(nullptr)
>    , httpEquivState(HTTP_EQUIV_NOT_SEEN)
>    , treeBuilder(tb)
> +  , mEncoding(nullptr)

Please file a follow-up to make sure this doesn't get overwritten the next time the parser is retranslated.
Attachment #8881094 - Flags: review?(hsivonen) → review+
(Assignee)

Updated

a year ago
Blocks: 1376154
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
mozreview-review-reply
Comment on attachment 8881094 [details]
Bug 1373984 - Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*>.

https://reviewboard.mozilla.org/r/152408/#review157480

> No need to use a string here anymore, AFAICT.

Good catch. Changed this to Encoding and UTF_8_ENCODING.

> Why not `ForName`?

Oh, .hintCharacterSet is already doing label resolution. Changed this to ForName.

> Hasn't this already gone through label resolution on the doc shell level?

I didn't read nsDocumentViewer::SetForceCharacterSet. Changed this to ForName.

> Why not `ForName`?

nsIEditor.documentCharacterSet is scriptable. If I change this to ForName, editor/libeditor/tests/test_documentCharacterSet.html will crash. It sets .documentCharacterSet from scripts and it will be used in the charset attribute of the meta element, so we need label resolution and we don't have to accept "replacement" here.

I added a comment to crarify.

> Please file a follow-up to make sure this doesn't get overwritten the next time the parser is retranslated.

Filed bug 1376154.

Comment 5

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/77af189b5c49
Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*>. r=hsivonen
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1376164
(Assignee)

Comment 8

a year ago
X86 calling conventions must die...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bdc873ffeb4712e26e3a8114d7154e4decea86a
Flags: needinfo?(VYV03354)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8881103 - Attachment is obsolete: true
Attachment #8881103 - Flags: review?(hsivonen)

Comment 10

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/7235d05662b0
Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*>. r=hsivonen
Backed out for Android build bustage:

https://hg.mozilla.org/integration/autoland/rev/e29483674e5a0f3e33bd41a24417bfd28a16861b

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7235d05662b03741f84c383534a46fecc2075d58&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=109801919&repo=autoland

[task 2017-06-25T16:24:36.816613Z] 16:24:36     INFO -  /home/worker/workspace/build/src/sccache2/sccache /home/worker/workspace/build/src/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -std=gnu++11 -o Unified_cpp_gfx_angle0.o -c -I/home/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/home/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /home/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -D_CRT_SECURE_NO_DEPRECATE -D_HAS_EXCEPTIONS=0 -D_SECURE_SCL=0 -DANGLE_ENABLE_D3D9 -DANGLE_SKIP_DXGI_1_2_CHECK -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL1 -DANGLE_NO_EXCEPTIONS -DGL_APICALL= -DGL_GLEXT_PROTOTYPES= -DEGLAPI= -DNOASSERT_UNIMPLEMENTED=0 -DANGLE_ENABLE_HLSL=1 -DANGLE_ENABLE_GLSL=1 -DANGLE_ENABLE_ESSL=1 -DANGLE_ENABLE_KEYEDMUTEX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/home/worker/workspace/build/src/gfx/angle -I/home/worker/workspace/build/src/obj-firefox/gfx/angle -I/home/worker/workspace/build/src/gfx/angle/include -I/home/worker/workspace/build/src/gfx/angle/src -I/home/worker/workspace/build/src/gfx/angle/src/common/third_party/numerics -I/home/worker/workspace/build/src/obj-firefox/dist/include  -I/home/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/home/worker/workspace/build/src/obj-firefox/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include /home/worker/workspace/build/src/obj-firefox/mozilla-config.h -MD -MP -MF .deps/Unified_cpp_gfx_angle0.o.pp -idirafter /home/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include   -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -fno-short-enums -fno-exceptions -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -mno-unaligned-access -I/home/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/libcxx/include -I/home/worker/workspace/build/src/android-ndk/sources/android/support/include -I/home/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -D_GLIBCXX_USE_CXX11_ABI=0 -pipe  -g -freorder-blocks -fno-reorder-functions -Os -funwind-tables -Wno-attributes -Wno-shadow -Wno-sign-compare -Wno-unknown-pragmas -Wno-unreachable-code -Wno-shadow-compatible-local -Wno-shadow-local  /home/worker/workspace/build/src/obj-firefox/gfx/angle/Unified_cpp_gfx_angle0.cpp
[task 2017-06-25T16:24:36.831999Z] 16:24:36     INFO -  cc1plus: error: to generate dependencies you must specify either -M or -MM
[task 2017-06-25T16:24:36.832592Z] 16:24:36     INFO -  /home/worker/workspace/build/src/config/rules.mk:1010: recipe for target 'Unified_cpp_parser_html0.o' failed
[task 2017-06-25T16:24:36.833326Z] 16:24:36     INFO -  gmake[5]: *** [Unified_cpp_parser_html0.o] Error 1
[task 2017-06-25T16:24:36.833686Z] 16:24:36     INFO -  gmake[5]: *** Waiting for unfinished jobs....
Flags: needinfo?(VYV03354)
(Assignee)

Comment 12

a year ago
Apparently Android builds dislikes using |nsIContentSink::Encoding;| and |nsIContentSink::NotNull;|
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a85ddf5755dedeb22a5447447933e8fe7a8ffe8e
Flags: needinfo?(VYV03354)
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/64412d8b6f4b
Turn nsIDocument::mCharacterSet into mozilla::NotNull<const mozilla::Encoding*>. r=hsivonen
Were the BoxObject and ImageLayer changes intentionally part of the bustage fix to make the namespaces work? (The ImageLayer change doesn't looks like a mere bustage fix.)
Flags: needinfo?(VYV03354)

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64412d8b6f4b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 17

a year ago
(In reply to Henri Sivonen (:hsivonen) from comment #15)
> Were the BoxObject and ImageLayer changes intentionally part of the bustage
> fix to make the namespaces work? (The ImageLayer change doesn't looks like a
> mere bustage fix.)

The MozReview interdiff is fundamentally broken (bug 1233076). The only actual my change is:
-  using nsIContentSink::Encoding;
-  using nsIContentSink::NotNull;
+  using Encoding = mozilla::Encoding;
+  template <typename T> using NotNull = mozilla::NotNull<T>;
Flags: needinfo?(VYV03354)

Updated

8 months ago
Depends on: 1418438
You need to log in before you can comment on or make changes to this bug.